Re: [RESEND PATCH 1/1] media: Use common test pattern menu entries

[Luca Ceresoli]

Hi Sakari, Bingbu,

On 27/11/18 10:34, Sakari Ailus wrote:
> While the test pattern menu itself is not standardised, many devices
> support the same test patterns. Aligning the menu entries helps the user
> space to use the interface, and adding macros for the menu entry strings
> helps to keep them aligned.
> 
> Signed-off-by: Sakari Ailus 
> ---
> Fixed Andy's email.
> 
>  drivers/media/i2c/imx258.c | 10 +-
>  drivers/media/i2c/imx319.c | 10 +-
>  drivers/media/i2c/imx355.c | 10 +-
>  drivers/media/i2c/ov2640.c |  4 ++--
>  drivers/media/i2c/smiapp/smiapp-core.c | 10 +-
>  include/uapi/linux/v4l2-controls.h |  5 +
>  6 files changed, 27 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index f86ae18bc104..c795d4c4c0e4 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -498,11 +498,11 @@ static const struct imx258_reg mode_1048_780_regs[] = {
>  };
>  
>  static const char * const imx258_test_pattern_menu[] = {
> - "Disabled",
> - "Solid Colour",
> - "Eight Vertical Colour Bars",
> - "Colour Bars With Fade to Grey",
> - "Pseudorandom Sequence (PN9)",
> + V4L2_TEST_PATTERN_DISABLED,
> + V4L2_TEST_PATTERN_SOLID_COLOUR,
> + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
> + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
> + V4L2_TEST_PATTERN_PN9,
>  };
>  
>  /* Configurations for supported link frequencies */
> diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
> index 17c2e4b41221..eddaf69a67b6 100644
> --- a/drivers/media/i2c/imx319.c
> +++ b/drivers/media/i2c/imx319.c
> @@ -1647,11 +1647,11 @@ static const struct imx319_reg mode_1280x720_regs[] = 
> {
>  };
>  
>  static const char * const imx319_test_pattern_menu[] = {
> - "Disabled",
> - "Solid Colour",
> - "Eight Vertical Colour Bars",
> - "Colour Bars With Fade to Grey",
> - "Pseudorandom Sequence (PN9)",
> + V4L2_TEST_PATTERN_DISABLED,
> + V4L2_TEST_PATTERN_SOLID_COLOUR,
> + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
> + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
> + V4L2_TEST_PATTERN_PN9,
>  };
>  
>  /* supported link frequencies */
> diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
> index bed293b60e50..824d07156f9c 100644
> --- a/drivers/media/i2c/imx355.c
> +++ b/drivers/media/i2c/imx355.c
> @@ -875,11 +875,11 @@ static const struct imx355_reg mode_820x616_regs[] = {
>  };
>  
>  static const char * const imx355_test_pattern_menu[] = {
> - "Disabled",
> - "Solid Colour",
> - "Eight Vertical Colour Bars",
> - "Colour Bars With Fade to Grey",
> - "Pseudorandom Sequence (PN9)",
> + V4L2_TEST_PATTERN_DISABLED,
> + V4L2_TEST_PATTERN_SOLID_COLOUR,
> + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
> + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
> + V4L2_TEST_PATTERN_PN9,
>  };
>  
>  /* supported link frequencies */
> diff --git a/drivers/media/i2c/ov2640.c b/drivers/media/i2c/ov2640.c
> index 5d2d6735cc78..507ec7176a7d 100644
> --- a/drivers/media/i2c/ov2640.c
> +++ b/drivers/media/i2c/ov2640.c
> @@ -707,8 +707,8 @@ static int ov2640_reset(struct i2c_client *client)
>  }
>  
>  static const char * const ov2640_test_pattern_menu[] = {
> - "Disabled",
> - "Eight Vertical Colour Bars",
> + V4L2_TEST_PATTERN_DISABLED,
> + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
>  };
>  
>  /*
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c 
> b/drivers/media/i2c/smiapp/smiapp-core.c
> index 58a45c353e27..f6a92b9f178c 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -409,11 +409,11 @@ static void smiapp_update_mbus_formats(struct 
> smiapp_sensor *sensor)
>  }
>  
>  static const char * const smiapp_test_patterns[] = {
> - "Disabled",
> - "Solid Colour",
> - "Eight Vertical Colour Bars",
> - "Colour Bars With Fade to Grey",
> - "Pseudorandom Sequence (PN9)",
> + V4L2_TEST_PATTERN_DISABLED,
> + V4L2_TEST_PATTERN_SOLID_COLOUR,
> + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS,
> + V4L2_TEST_PATTERN_8_VERT_COLOUR_BARS_FADE_TO_GREY,
> + V4L2_TEST_PATTERN_PN9,
>  };
>  
>  static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
> diff --git a/include/uapi/linux/v4l2-controls.h 
> b/include/uapi/linux/v4l2-controls.h
> index 998983a6e6b7..a74ff6f1ac88 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1014,6 +1014,11 @@ enum v4l2_jpeg_chroma_subsampling {
>  #define V4L2_CID_LINK_FREQ   (V4L2_CID_IMAGE_PROC_CLASS_BASE 
> + 1)
>  #define V4L2_CID_PIXEL_RATE  (V4L2_CID_IMAGE_PROC_CLASS_BASE 
> + 2)
>  #define V4L2_CID_TEST_PATTERN
> (V4L2_CID_IMAGE_PROC_CLASS_BASE + 3)
> +#define V4L2_TEST_PATTERN_DISABLED   "Disabled"
> +#define 

Re: [PATCH] media: unify some sony camera sensors pattern naming

[Luca Ceresoli]

Hi Bingbu,

On 27/11/18 09:55, Bingbu Cao wrote:
> 
> 
> On 11/27/2018 04:05 PM, Luca Ceresoli wrote:
>> Hi Bingbu,
>>
>> On 27/11/18 05:01, bingbu@intel.com wrote:
>>> From: Bingbu Cao 
>>>
>>> Some Sony camera sensors have same test pattern
>>> definitions, this patch unify the pattern naming
>>> to make it more clear to the userspace.
>>>
>>> Suggested-by: Sakari Ailus 
>>> Signed-off-by: Bingbu Cao 
>>> ---
>>>   drivers/media/i2c/imx258.c | 8 
>>>   drivers/media/i2c/imx319.c | 8 
>>>   drivers/media/i2c/imx355.c | 8 
>>>   3 files changed, 12 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
>>> index 31a1e2294843..a8a2880c6b4e 100644
>>> --- a/drivers/media/i2c/imx258.c
>>> +++ b/drivers/media/i2c/imx258.c
>>> @@ -504,10 +504,10 @@ struct imx258_mode {
>>>     static const char * const imx258_test_pattern_menu[] = {
>>>   "Disabled",
>>> -    "Color Bars",
>>> -    "Solid Color",
>>> -    "Grey Color Bars",
>>> -    "PN9"
>>> +    "Solid Colour",
>>> +    "Eight Vertical Colour Bars",
>>> +    "Colour Bars With Fade to Grey",
>>> +    "Pseudorandom Sequence (PN9)",
>> I had a look at imx274, it has many more values but definitely some
>> could be unified too.
>>
>> However I noticed something strange in that driver: The "Horizontal
>> Color Bars" pattern has vertical bars, side-by-side, as in .
>> "Vertical Color Bars" are one on top of the other, as in ==. Is it just
>> me crazy, or are they swapped?
> Luca, thanks for your review.
> I do not have the manual of imx274.
>  should be the 'Vertical Color Bars' without any rotation process.
> If not, I think the definitions there are swapped.

Definitely. I'll send a patch for imx274.

-- 
Luca

Re: [PATCH] media: unify some sony camera sensors pattern naming

[Luca Ceresoli]

Hi Bingbu,

On 27/11/18 05:01, bingbu@intel.com wrote:
> From: Bingbu Cao 
> 
> Some Sony camera sensors have same test pattern
> definitions, this patch unify the pattern naming
> to make it more clear to the userspace.
> 
> Suggested-by: Sakari Ailus 
> Signed-off-by: Bingbu Cao 
> ---
>  drivers/media/i2c/imx258.c | 8 
>  drivers/media/i2c/imx319.c | 8 
>  drivers/media/i2c/imx355.c | 8 
>  3 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index 31a1e2294843..a8a2880c6b4e 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -504,10 +504,10 @@ struct imx258_mode {
>  
>  static const char * const imx258_test_pattern_menu[] = {
>   "Disabled",
> - "Color Bars",
> - "Solid Color",
> - "Grey Color Bars",
> - "PN9"
> + "Solid Colour",
> + "Eight Vertical Colour Bars",
> + "Colour Bars With Fade to Grey",
> + "Pseudorandom Sequence (PN9)",

I had a look at imx274, it has many more values but definitely some
could be unified too.

However I noticed something strange in that driver: The "Horizontal
Color Bars" pattern has vertical bars, side-by-side, as in .
"Vertical Color Bars" are one on top of the other, as in ==. Is it just
me crazy, or are they swapped?

Only one minor nitpick about your patch. The USA spelling "color" seems
a lot more frequent in the kernel sources than the UK "colour", so it's
probably better to be consistent.

-- 
Luca

[PATCH v6 2/2] media: imx274: add cropping support via SELECTION API

[Luca Ceresoli]

Currently this driver does not support cropping. The supported modes
are the following, all capturing the entire area:

 - 3840x2160, 1:1 binning (native sensor resolution)
 - 1920x1080, 2:1 binning
 - 1280x720,  3:1 binning

The VIDIOC_SUBDEV_S_FMT ioctl chooses among these 3 configurations the
one that matches the requested format.

Add cropping support via VIDIOC_SUBDEV_S_SELECTION: with target
V4L2_SEL_TGT_CROP to choose the captured area, with
V4L2_SEL_TGT_COMPOSE to choose the output resolution.

To maintain backward compatibility we also allow setting the compose
format via VIDIOC_SUBDEV_S_FMT. To obtain this, compose rect and
output format are computed in the common helper function
__imx274_change_compose(), which sets both to the same width/height
values.

Cropping also calls __imx274_change_compose() whenever cropping rect
size changes in order to reset the compose rect (and output format
size) for 1:1 binning.

Also rename enum imx274_mode to imx274_binning (and its values from
IMX274_MODE_BINNING_* to IMX274_BINNING_*). Without cropping, the two
naming are equivalent. With cropping, the resolution could be
different, e.g. using 2:1 binning mode to crop 1200x960 and output a
600x480 format. Using binning in the names avoids any
misunderstanding. For the same reason, replace the 'size' field in
struct imx274_frmfmt with 'bin_ratio'.

Signed-off-by: Luca Ceresoli 
Cc: Sakari Ailus 

---
Changed v5 -> v6:
 - simplify last call to imx274_write_mbreg() (Sakari)
 - fix typo in comment
 - remove unused variable

Changed v4 -> v5:
 - use imx274_write_mbreg, not prepare_reg (it doesn't exist in v5)
 - order #includes alphabetically (Sakari)
 - rename imx274_mode to imx274_binning and
   IMX274_MODE_BINNING_* to IMX274_BINNING_* (Sakari)
 - fix doc syntax (Sakari)
 - remove debug prints that should not be in individual drivers (Sakari)
 - honor the GE/LE selection flags (algorithm inspired to smiapp) (Sakari)
 - remove the compose rect from struct stimx274; it duplicates info
   that we already store in stimx274.format
 - minor cleanups

Changed v3 -> v4:
 - Set the binning via the SELECTION API (COMPOSE rect), but still
   allow using VIDIOC_SUBDEV_S_FMT for backward compatibility.
 - rename imx274_set_trimming -> imx274_apply_trimming for clarity

Changed v2 -> v3:
 - Remove hard-coded HMAX reg setting from all modes, not only the
   first one. HMAX is always computed and set in s_stream now.
 - Reword commit log message.

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 464 +
 1 file changed, 373 insertions(+), 91 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 093c4587832a..2d859f46ce13 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -5,6 +5,7 @@
  *
  * Leon Luo 
  * Edwin Zou 
+ * Luca Ceresoli 
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -25,6 +26,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -74,7 +76,7 @@
  */
 #define IMX274_MIN_EXPOSURE_TIME   (4 * 260 / 72)
 
-#define IMX274_DEFAULT_MODEIMX274_MODE_3840X2160
+#define IMX274_DEFAULT_MODEIMX274_BINNING_OFF
 #define IMX274_MAX_WIDTH   (3840)
 #define IMX274_MAX_HEIGHT  (2160)
 #define IMX274_MAX_FRAME_RATE  (120)
@@ -111,6 +113,20 @@
 #define IMX274_SHR_REG_LSB 0x300C /* SHR */
 #define IMX274_SVR_REG_MSB 0x300F /* SVR */
 #define IMX274_SVR_REG_LSB 0x300E /* SVR */
+#define IMX274_HTRIM_EN_REG0x3037
+#define IMX274_HTRIM_START_REG_LSB 0x3038
+#define IMX274_HTRIM_START_REG_MSB 0x3039
+#define IMX274_HTRIM_END_REG_LSB   0x303A
+#define IMX274_HTRIM_END_REG_MSB   0x303B
+#define IMX274_VWIDCUTEN_REG   0x30DD
+#define IMX274_VWIDCUT_REG_LSB 0x30DE
+#define IMX274_VWIDCUT_REG_MSB 0x30DF
+#define IMX274_VWINPOS_REG_LSB 0x30E0
+#define IMX274_VWINPOS_REG_MSB 0x30E1
+#define IMX274_WRITE_VSIZE_REG_LSB 0x3130
+#define IMX274_WRITE_VSIZE_REG_MSB 0x3131
+#define IMX274_Y_OUT_SIZE_REG_LSB  0x3132
+#define IMX274_Y_OUT_SIZE_REG_MSB  0x3133
 #define IMX274_VMAX_REG_1  0x30FA /* VMAX, MSB */
 #define IMX274_VMAX_REG_2  0x30F9 /* VMAX */
 #define IMX274_VMAX_REG_3  0x30F8 /* VMAX, LSB */
@@ -140,10 +156,10 @@ static const struct regmap_config imx274_regmap_config = {
.cache_type = REGCACHE_RBTREE,
 };
 
-enum imx274_mode {
-   IMX274_MODE_3840X2160,
-   IMX274_MODE_1920X1080,
-   IMX274_MODE_12

[PATCH v6 1/2] media: imx274: use regmap_bulk_write to write multybyte registers

[Luca Ceresoli]

Currently 2-bytes and 3-bytes registers are set by very similar
functions doing the needed shift & mask manipulation, followed by very
similar for loops setting one byte at a time over I2C.

Replace all of this code by a unique helper function that calls
regmap_bulk_write(), which has two advantages:
 - sets all the bytes in a unique I2C transaction
 - removes lots of now unused code.

Signed-off-by: Luca Ceresoli 
Cc: Sakari Ailus 

---
Changed v5 -> v6:
 - fix warning on debug print on 32-bit machines (reported by the
   kbuild test robot)
 - fix line above 80 chars

Changed v4 -> v5:
 - completely replace the former patch adding prepare_regs() by
   directly calling regmap_bulk_write() (Sakari)

Changed v3 -> v4: nothing

Changed v2 -> v3:
 - minor reformatting in prepare_reg() documentation

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 102 ++---
 1 file changed, 39 insertions(+), 63 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index edca63eaea9b..093c4587832a 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -690,6 +690,35 @@ static inline int imx274_write_reg(struct stimx274 *priv, 
u16 addr, u8 val)
return err;
 }
 
+/**
+ * Write a multibyte register.
+ *
+ * Uses a bulk write where possible.
+ *
+ * @priv: Pointer to device structure
+ * @addr: Address of the LSB register.  Other registers must be
+ *consecutive, least-to-most significant.
+ * @val: Value to be written to the register (cpu endianness)
+ * @nbytes: Number of bits to write (range: [1..3])
+ */
+static int imx274_write_mbreg(struct stimx274 *priv, u16 addr, u32 val,
+ size_t nbytes)
+{
+   __le32 val_le = cpu_to_le32(val);
+   int err;
+
+   err = regmap_bulk_write(priv->regmap, addr, _le, nbytes);
+   if (err)
+   dev_err(>client->dev,
+   "%s : i2c bulk write failed, %x = %x (%zu bytes)\n",
+   __func__, addr, val, nbytes);
+   else
+   dev_dbg(>client->dev,
+   "%s : addr 0x%x, val=0x%x (%zu bytes)\n",
+   __func__, addr, val, nbytes);
+   return err;
+}
+
 /*
  * Set mode registers to start stream.
  * @priv: Pointer to device structure
@@ -1163,15 +1192,6 @@ static int imx274_set_digital_gain(struct stimx274 
*priv, u32 dgain)
reg_val & IMX274_MASK_LSB_4_BITS);
 }
 
-static inline void imx274_calculate_gain_regs(struct reg_8 regs[2], u16 gain)
-{
-   regs->addr = IMX274_ANALOG_GAIN_ADDR_MSB;
-   regs->val = (gain >> IMX274_SHIFT_8_BITS) & IMX274_MASK_LSB_3_BITS;
-
-   (regs + 1)->addr = IMX274_ANALOG_GAIN_ADDR_LSB;
-   (regs + 1)->val = (gain) & IMX274_MASK_LSB_8_BITS;
-}
-
 /*
  * imx274_set_gain - Function called when setting gain
  * @priv: Pointer to device structure
@@ -1185,10 +1205,8 @@ static inline void imx274_calculate_gain_regs(struct 
reg_8 regs[2], u16 gain)
  */
 static int imx274_set_gain(struct stimx274 *priv, struct v4l2_ctrl *ctrl)
 {
-   struct reg_8 reg_list[2];
int err;
u32 gain, analog_gain, digital_gain, gain_reg;
-   int i;
 
gain = (u32)(ctrl->val);
 
@@ -1229,14 +1247,10 @@ static int imx274_set_gain(struct stimx274 *priv, 
struct v4l2_ctrl *ctrl)
if (gain_reg > IMX274_GAIN_REG_MAX)
gain_reg = IMX274_GAIN_REG_MAX;
 
-   imx274_calculate_gain_regs(reg_list, (u16)gain_reg);
-
-   for (i = 0; i < ARRAY_SIZE(reg_list); i++) {
-   err = imx274_write_reg(priv, reg_list[i].addr,
-  reg_list[i].val);
-   if (err)
-   goto fail;
-   }
+   err = imx274_write_mbreg(priv, IMX274_ANALOG_GAIN_ADDR_LSB, gain_reg,
+2);
+   if (err)
+   goto fail;
 
if (IMX274_GAIN_CONST - gain_reg == 0) {
err = -EINVAL;
@@ -1258,16 +1272,6 @@ static int imx274_set_gain(struct stimx274 *priv, struct 
v4l2_ctrl *ctrl)
return err;
 }
 
-static inline void imx274_calculate_coarse_time_regs(struct reg_8 regs[2],
-u32 coarse_time)
-{
-   regs->addr = IMX274_SHR_REG_MSB;
-   regs->val = (coarse_time >> IMX274_SHIFT_8_BITS)
-   & IMX274_MASK_LSB_8_BITS;
-   (regs + 1)->addr = IMX274_SHR_REG_LSB;
-   (regs + 1)->val = (coarse_time) & IMX274_MASK_LSB_8_BITS;
-}
-
 /*
  * imx274_set_coarse_time - Function called when setting SHR value
  * @priv: Pointer to device structure
@@ -1279,10 +1283,8 @@ static inline void 
imx274_calculate_coarse_time_regs(struct reg_8 regs[2],
  */
 static int imx274_set_coarse_time(struct stimx274 *priv, u32 *val)

[PATCH v6 0/2] media: imx274: cleanups, improvements and SELECTION API support

[Luca Ceresoli]

Hi,

this patchset introduces cropping support for the Sony IMX274 sensor
using the SELECTION API.

Changes since v5 are only minor fixes and cleanups.

Patch 1 introduces a helper to greatly simplify the code to write a
multibyte register.

Patch 2 implements the set_selection pad operation for cropping
(V4L2_SEL_TGT_CROP) and binning (V4L2_SEL_TGT_COMPOSE).

Usage examples:

 * Capture the entire 4K area, downscaled to 1080p with 2:1 binning:
   media-ctl -V '"IMX274":0[crop:(0,0)/3840x2160]'
   media-ctl -V '"IMX274":0[compose:(0,0)/1920x1080]'

 * Capture the central 1080p area (no binning):
   media-ctl -V '"IMX274":0[crop:(960,540)/1920x1080]'
   (this also sets the compose and fmt rects to 1920x1080)

Regards,
Luca


Luca Ceresoli (2):
  media: imx274: use regmap_bulk_write to write multybyte registers
  media: imx274: add cropping support via SELECTION API

 drivers/media/i2c/imx274.c | 566 +++--
 1 file changed, 412 insertions(+), 154 deletions(-)

-- 
2.17.1

[PATCH v4 7/8] media: imx274: fix typo

[Luca Ceresoli]

pd -> pad

Signed-off-by: Luca Ceresoli 
Cc: Sakari Ailus 

---
Changed v3 -> v4: nothing
---
 drivers/media/i2c/imx274.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index e5ba19b97083..2c13961e9764 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -544,7 +544,7 @@ struct imx274_ctrls {
 /*
  * struct stim274 - imx274 device structure
  * @sd: V4L2 subdevice structure
- * @pd: Media pad structure
+ * @pad: Media pad structure
  * @client: Pointer to I2C client
  * @ctrls: imx274 control structure
  * @format: V4L2 media bus frame format structure
-- 
2.7.4

[PATCH v4 0/8] media: imx274: cleanups, improvements and SELECTION API support

[Luca Ceresoli]

Hi,

this patchset introduces cropping support for the Sony IMX274 sensor
using the SELECTION API.

With respect to v3, this version uses the SELECTION API with taget
V4L2_SEL_TGT_COMPOSE to change the output resolution. This is the
recommended API for cropping + downscaling. However for backward
compatibility the set_format callback is still supported and is
equivalent to setting the compose rect as far as resolutions are
concerned.

Patches 1-5 are overall improvements and restructuring, mostly useful
to implement the SELECTION API in a clean way.

Patch 6 introduces a helper to allow setting many registers computed
at runtime in a straightforward way. This would not have been very
useful before because all long register write sequences came from
const tables, but it's definitely a must for the cropping code where
several register values are computed at runtime.

Patch 7 is new in this series, it's a trivial typo fix that can be
applied independently.

Patch 8 implements the set_selection pad operation for cropping
(V4L2_SEL_TGT_CROP) and binning (V4L2_SEL_TGT_COMPOSE). The most
tricky part was respecting all the device constraints on the
horizontal crop.

Usage examples:

 * Capture the entire 4K area, downscaled to 1080p with 2:1 binning:
   media-ctl -V '"IMX274":0[crop:(0,0)/3840x2160]'
   media-ctl -V '"IMX274":0[compose:(0,0)/1920x1080]'

 * Capture the central 1080p area (no binning):
   media-ctl -V '"IMX274":0[crop:(960,540)/1920x1080]'
   (this also sets the compose and fmt rects to 1920x1080)

Regards,
Luca


Luca Ceresoli (8):
  media: imx274: initialize format before v4l2 controls
  media: imx274: consolidate per-mode data in imx274_frmfmt
  media: imx274: get rid of mode_index
  media: imx274: actually use IMX274_DEFAULT_MODE
  media: imx274: simplify imx274_write_table()
  media: imx274: add helper function to fill a reg_8 table chunk
  media: imx274: fix typo
  media: imx274: add SELECTION support for cropping

 drivers/media/i2c/imx274.c | 686 ++---
 1 file changed, 461 insertions(+), 225 deletions(-)

-- 
2.7.4

[PATCH v4 3/8] media: imx274: get rid of mode_index

[Luca Ceresoli]

After restructuring struct imx274_frmfmt, the mode_index field is
still in use only for two dev_dbg() calls in imx274_s_stream(). Let's
remove it and avoid duplicated information.

Replacing the first usage requires some rather annoying but trivial
pointer math. The other one can be removed entirely since it would
print the same value anyway.

Signed-off-by: Luca Ceresoli 
Cc: Sakari Ailus 

---
Changed v3 -> v4: nothing

Changed v2 -> v3:
 - really fix dev_dbg() format mismatch warning for both 32 and 64 bit

Changed v1 -> v2:
 - add "media: " prefix to commit message
 - fix dev_dbg() format mismatch warning
   ("warning: format ‘%ld’ expects argument of type ‘long int’, but argument 6 
has type ‘int’")
 - slightly improve commit message
---
 drivers/media/i2c/imx274.c | 15 +--
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 2ec31ae4e60d..f075715ffced 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -553,8 +553,6 @@ struct imx274_ctrls {
  * @reset_gpio: Pointer to reset gpio
  * @lock: Mutex structure
  * @mode: Parameters for the selected readout mode
- *(points to imx274_formats[mode_index])
- * @mode_index: Resolution mode index
  */
 struct stimx274 {
struct v4l2_subdev sd;
@@ -567,7 +565,6 @@ struct stimx274 {
struct gpio_desc *reset_gpio;
struct mutex lock; /* mutex lock for operations */
const struct imx274_frmfmt *mode;
-   u32 mode_index;
 };
 
 /*
@@ -880,7 +877,6 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
index = 0;
}
 
-   imx274->mode_index = index;
imx274->mode = _formats[index];
 
if (fmt->width > IMX274_MAX_WIDTH)
@@ -1028,8 +1024,9 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
struct stimx274 *imx274 = to_imx274(sd);
int ret = 0;
 
-   dev_dbg(>client->dev, "%s : %s, mode index = %d\n", __func__,
-   on ? "Stream Start" : "Stream Stop", imx274->mode_index);
+   dev_dbg(>client->dev, "%s : %s, mode index = %td\n", __func__,
+   on ? "Stream Start" : "Stream Stop",
+   imx274->mode - _formats[0]);
 
mutex_lock(>lock);
 
@@ -1068,8 +1065,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
}
 
mutex_unlock(>lock);
-   dev_dbg(>client->dev,
-   "%s : Done: mode = %d\n", __func__, imx274->mode_index);
+   dev_dbg(>client->dev, "%s : Done\n", __func__);
return 0;
 
 fail:
@@ -1625,8 +1621,7 @@ static int imx274_probe(struct i2c_client *client,
mutex_init(>lock);
 
/* initialize format */
-   imx274->mode_index = IMX274_MODE_3840X2160;
-   imx274->mode = _formats[imx274->mode_index];
+   imx274->mode = _formats[IMX274_MODE_3840X2160];
imx274->format.width = imx274->mode->size.width;
imx274->format.height = imx274->mode->size.height;
imx274->format.field = V4L2_FIELD_NONE;
-- 
2.7.4

[PATCH v4 1/8] media: imx274: initialize format before v4l2 controls

[Luca Ceresoli]

The current probe function calls v4l2_ctrl_handler_setup() before
initializing the format info. This triggers call paths such as:
imx274_probe -> v4l2_ctrl_handler_setup -> imx274_s_ctrl ->
imx274_set_exposure, where priv->mode_index is accessed before being
assigned.

This is wrong but does not trigger a visible bug because priv is
zero-initialized and 0 is the default value for priv->mode_index. But
this would become a crash in follow-up commits when mode_index is
replaced by a pointer that must always be valid.

Fix the bug before it shows up by initializing struct members early.

Signed-off-by: Luca Ceresoli 
Cc: Sakari Ailus 

---
Changed v3 -> v4: nothing

Changed v2 -> v3: nothing

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 63fb94e7da37..8a8a11b8d75d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1632,6 +1632,16 @@ static int imx274_probe(struct i2c_client *client,
 
mutex_init(>lock);
 
+   /* initialize format */
+   imx274->mode_index = IMX274_MODE_3840X2160;
+   imx274->format.width = imx274_formats[0].size.width;
+   imx274->format.height = imx274_formats[0].size.height;
+   imx274->format.field = V4L2_FIELD_NONE;
+   imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
+   imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
+   imx274->frame_interval.numerator = 1;
+   imx274->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
+
/* initialize regmap */
imx274->regmap = devm_regmap_init_i2c(client, _regmap_config);
if (IS_ERR(imx274->regmap)) {
@@ -1720,16 +1730,6 @@ static int imx274_probe(struct i2c_client *client,
goto err_ctrls;
}
 
-   /* initialize format */
-   imx274->mode_index = IMX274_MODE_3840X2160;
-   imx274->format.width = imx274_formats[0].size.width;
-   imx274->format.height = imx274_formats[0].size.height;
-   imx274->format.field = V4L2_FIELD_NONE;
-   imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
-   imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
-   imx274->frame_interval.numerator = 1;
-   imx274->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
-
/* load default control values */
ret = imx274_load_default(imx274);
if (ret) {
-- 
2.7.4

[PATCH v4 5/8] media: imx274: simplify imx274_write_table()

[Luca Ceresoli]

imx274_write_table() is a mere wrapper (and the only user) to
imx274_regmap_util_write_table_8(). Remove this useless indirection by
merging the two functions into one.

Also get rid of the wait_ms_addr and end_addr parameters since it does
not make any sense to give them any values other than
IMX274_TABLE_WAIT_MS and IMX274_TABLE_END.

Signed-off-by: Luca Ceresoli 
Cc: Sakari Ailus 

---
Changed v3 -> v4: nothing

Changed v2 -> v3: nothing

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 28 ++--
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index ceeec97cd330..48343c2ade83 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -597,20 +597,18 @@ static inline struct stimx274 *to_imx274(struct 
v4l2_subdev *sd)
 }
 
 /*
- * imx274_regmap_util_write_table_8 - Function for writing register table
- * @regmap: Pointer to device reg map structure
- * @table: Table containing register values
- * @wait_ms_addr: Flag for performing delay
- * @end_addr: Flag for incating end of table
+ * Writing a register table
+ *
+ * @priv: Pointer to device
+ * @table: Table containing register values (with optional delays)
  *
  * This is used to write register table into sensor's reg map.
  *
  * Return: 0 on success, errors otherwise
  */
-static int imx274_regmap_util_write_table_8(struct regmap *regmap,
-   const struct reg_8 table[],
-   u16 wait_ms_addr, u16 end_addr)
+static int imx274_write_table(struct stimx274 *priv, const struct reg_8 
table[])
 {
+   struct regmap *regmap = priv->regmap;
int err = 0;
const struct reg_8 *next;
u8 val;
@@ -622,8 +620,8 @@ static int imx274_regmap_util_write_table_8(struct regmap 
*regmap,
 
for (next = table;; next++) {
if ((next->addr != range_start + range_count) ||
-   (next->addr == end_addr) ||
-   (next->addr == wait_ms_addr) ||
+   (next->addr == IMX274_TABLE_END) ||
+   (next->addr == IMX274_TABLE_WAIT_MS) ||
(range_count == max_range_vals)) {
if (range_count == 1)
err = regmap_write(regmap,
@@ -642,10 +640,10 @@ static int imx274_regmap_util_write_table_8(struct regmap 
*regmap,
range_count = 0;
 
/* Handle special address values */
-   if (next->addr == end_addr)
+   if (next->addr == IMX274_TABLE_END)
break;
 
-   if (next->addr == wait_ms_addr) {
+   if (next->addr == IMX274_TABLE_WAIT_MS) {
msleep_range(next->val);
continue;
}
@@ -692,12 +690,6 @@ static inline int imx274_write_reg(struct stimx274 *priv, 
u16 addr, u8 val)
return err;
 }
 
-static int imx274_write_table(struct stimx274 *priv, const struct reg_8 
table[])
-{
-   return imx274_regmap_util_write_table_8(priv->regmap,
-   table, IMX274_TABLE_WAIT_MS, IMX274_TABLE_END);
-}
-
 /*
  * Set mode registers to start stream.
  * @priv: Pointer to device structure
-- 
2.7.4

[PATCH v4 6/8] media: imx274: add helper function to fill a reg_8 table chunk

[Luca Ceresoli]

Tables of struct reg_8 are used to simplify multi-byte register
assignment. However filling these snippets with values computed at
runtime is currently implemented by very similar functions doing the
needed shift & mask manipulation.

Replace all those functions with a unique helper function to fill
reg_8 tables in a simple and clean way.

Signed-off-by: Luca Ceresoli 
Cc: Sakari Ailus 

---
Changed v3 -> v4: nothing

Changed v2 -> v3:
 - minor reformatting in prepare_reg() documentation

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 90 --
 1 file changed, 55 insertions(+), 35 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 48343c2ade83..e5ba19b97083 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -597,6 +597,58 @@ static inline struct stimx274 *to_imx274(struct 
v4l2_subdev *sd)
 }
 
 /*
+ * Fill consecutive reg_8 items in order to set a multibyte register.
+ *
+ * The sensor has many 2-bytes registers and a 3-byte register. This
+ * simplifies code to set them by filling consecutive entries of a
+ * struct reg_8 table with the data to set a register.
+ *
+ * The number of table entries that is filled is the minimum needed
+ * for the given number of bits (i.e. nbits / 8, rounded up). If nbits
+ * is not a multiple of 8, extra bits in the most significant byte are
+ * zeroed.
+ *
+ * Example:
+ *   Calling prepare_reg([10], 0x3000, 0xcafe, 12) will set:
+ *   regs[10] = { .addr = 0x3000, .val = 0xfe }
+ *   regs[11] = { .addr = 0x3001, .val = 0x0a }
+ *
+ * @table_base: Pointer to the first reg_8 struct to be filled.  The
+ *  following entries will also be set, make sure they are
+ *  properly allocated!
+ * @addr_lsb: Address of the LSB register.  Other registers must be
+ *consecutive, least-to-most significant.
+ * @value: Value to be written to the register.
+ * @nbits: Number of bits to write (range: [1..24])
+ */
+static void prepare_reg(struct reg_8 *table_base,
+   u16 addr_lsb,
+   u32 value,
+   size_t nbits)
+{
+   struct reg_8 *cmd = table_base;
+   u16 addr = addr_lsb;
+   size_t bits; /* how many bits at this round */
+
+   WARN_ON(nbits > 24);
+
+   if (nbits > 24)
+   nbits = 24;
+
+   while (nbits > 0) {
+   bits = min_t(size_t, 8, nbits);
+
+   cmd->addr = addr;
+   cmd->val = value & ((1 << bits) - 1);
+
+   nbits -= bits;
+   value >>= 8;
+   addr++;
+   cmd++;
+   }
+}
+
+/*
  * Writing a register table
  *
  * @priv: Pointer to device
@@ -1163,15 +1215,6 @@ static int imx274_set_digital_gain(struct stimx274 
*priv, u32 dgain)
reg_val & IMX274_MASK_LSB_4_BITS);
 }
 
-static inline void imx274_calculate_gain_regs(struct reg_8 regs[2], u16 gain)
-{
-   regs->addr = IMX274_ANALOG_GAIN_ADDR_MSB;
-   regs->val = (gain >> IMX274_SHIFT_8_BITS) & IMX274_MASK_LSB_3_BITS;
-
-   (regs + 1)->addr = IMX274_ANALOG_GAIN_ADDR_LSB;
-   (regs + 1)->val = (gain) & IMX274_MASK_LSB_8_BITS;
-}
-
 /*
  * imx274_set_gain - Function called when setting gain
  * @priv: Pointer to device structure
@@ -1229,7 +1272,7 @@ static int imx274_set_gain(struct stimx274 *priv, struct 
v4l2_ctrl *ctrl)
if (gain_reg > IMX274_GAIN_REG_MAX)
gain_reg = IMX274_GAIN_REG_MAX;
 
-   imx274_calculate_gain_regs(reg_list, (u16)gain_reg);
+   prepare_reg(reg_list, IMX274_ANALOG_GAIN_ADDR_LSB, gain_reg, 11);
 
for (i = 0; i < ARRAY_SIZE(reg_list); i++) {
err = imx274_write_reg(priv, reg_list[i].addr,
@@ -1258,16 +1301,6 @@ static int imx274_set_gain(struct stimx274 *priv, struct 
v4l2_ctrl *ctrl)
return err;
 }
 
-static inline void imx274_calculate_coarse_time_regs(struct reg_8 regs[2],
-u32 coarse_time)
-{
-   regs->addr = IMX274_SHR_REG_MSB;
-   regs->val = (coarse_time >> IMX274_SHIFT_8_BITS)
-   & IMX274_MASK_LSB_8_BITS;
-   (regs + 1)->addr = IMX274_SHR_REG_LSB;
-   (regs + 1)->val = (coarse_time) & IMX274_MASK_LSB_8_BITS;
-}
-
 /*
  * imx274_set_coarse_time - Function called when setting SHR value
  * @priv: Pointer to device structure
@@ -1292,7 +1325,7 @@ static int imx274_set_coarse_time(struct stimx274 *priv, 
u32 *val)
goto fail;
 
/* prepare SHR registers */
-   imx274_calculate_coarse_time_regs(reg_list, coarse_time);
+   prepare_reg(reg_list, IMX274_SHR_REG_LSB, coarse_time, 16);
 
/* write to SHR registers */
for (i = 0; i < ARRAY_SIZE(reg_list); i++) {
@@ -1429,19 +146

[PATCH v4 8/8] media: imx274: add SELECTION support for cropping

[Luca Ceresoli]

Currently this driver does not support cropping. The supported modes
are the following, all capturing the entire area:

 - 3840x2160, 1:1 binning (native sensor resolution)
 - 1920x1080, 2:1 binning
 - 1280x720,  3:1 binning

The VIDIOC_SUBDEV_S_FMT ioctl chooses among these 3 configurations the
one that matches the requested format.

Add cropping support via VIDIOC_SUBDEV_S_SELECTION: with target
V4L2_SEL_TGT_CROP to choose the captured area, with
V4L2_SEL_TGT_COMPOSE to choose the output resolution.

To maintain backward compatibility we also allow setting the compose
format via VIDIOC_SUBDEV_S_FMT. To obtain this, compose rect and
output format are computed in the common helper function
__imx274_change_compose(), which sets both to the same width/height
values.

Cropping also calls __imx274_change_compose() whenever cropping rect
size changes in order to reset the compose rect (and output format
size) for 1:1 binning.

Also change the names in enum imx274_mode from being resolution-based
to being binning-based (e.g. from IMX274_MODE_1920X1080 to
IMX274_MODE_BINNING_2_1). Without cropping, the two naming are
equivalent. With cropping, the resolution could be different,
e.g. using 2:1 binning mode to crop 1200x960 and output a 600x480
format. Using binning in the names avoids any misunderstanding.  For
the same reason, replace the 'size' field in struct imx274_frmfmt with
'bin_ratio'.

Signed-off-by: Luca Ceresoli 
Cc: Sakari Ailus 

---
Changed v3 -> v4:
 - Set the binning via the SELECTION API (COMPOSE rect), but still
   allow using VIDIOC_SUBDEV_S_FMT for backward compatibility.
 - rename imx274_set_trimming -> imx274_apply_trimming for clarity

Changed v2 -> v3:
 - Remove hard-coded HMAX reg setting from all modes, not only the
   first one. HMAX is always computed and set in s_stream now.
 - Reword commit log message.

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 416 +++--
 1 file changed, 326 insertions(+), 90 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 2c13961e9764..8bfc20a46b3d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -5,6 +5,7 @@
  *
  * Leon Luo 
  * Edwin Zou 
+ * Luca Ceresoli 
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -19,6 +20,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -74,7 +76,7 @@
  */
 #define IMX274_MIN_EXPOSURE_TIME   (4 * 260 / 72)
 
-#define IMX274_DEFAULT_MODEIMX274_MODE_3840X2160
+#define IMX274_DEFAULT_MODEIMX274_MODE_BINNING_OFF
 #define IMX274_MAX_WIDTH   (3840)
 #define IMX274_MAX_HEIGHT  (2160)
 #define IMX274_MAX_FRAME_RATE  (120)
@@ -111,6 +113,20 @@
 #define IMX274_SHR_REG_LSB 0x300C /* SHR */
 #define IMX274_SVR_REG_MSB 0x300F /* SVR */
 #define IMX274_SVR_REG_LSB 0x300E /* SVR */
+#define IMX274_HTRIM_EN_REG0x3037
+#define IMX274_HTRIM_START_REG_LSB 0x3038
+#define IMX274_HTRIM_START_REG_MSB 0x3039
+#define IMX274_HTRIM_END_REG_LSB   0x303A
+#define IMX274_HTRIM_END_REG_MSB   0x303B
+#define IMX274_VWIDCUTEN_REG   0x30DD
+#define IMX274_VWIDCUT_REG_LSB 0x30DE
+#define IMX274_VWIDCUT_REG_MSB 0x30DF
+#define IMX274_VWINPOS_REG_LSB 0x30E0
+#define IMX274_VWINPOS_REG_MSB 0x30E1
+#define IMX274_WRITE_VSIZE_REG_LSB 0x3130
+#define IMX274_WRITE_VSIZE_REG_MSB 0x3131
+#define IMX274_Y_OUT_SIZE_REG_LSB  0x3132
+#define IMX274_Y_OUT_SIZE_REG_MSB  0x3133
 #define IMX274_VMAX_REG_1  0x30FA /* VMAX, MSB */
 #define IMX274_VMAX_REG_2  0x30F9 /* VMAX */
 #define IMX274_VMAX_REG_3  0x30F8 /* VMAX, LSB */
@@ -141,9 +157,9 @@ static const struct regmap_config imx274_regmap_config = {
 };
 
 enum imx274_mode {
-   IMX274_MODE_3840X2160,
-   IMX274_MODE_1920X1080,
-   IMX274_MODE_1280X720,
+   IMX274_MODE_BINNING_OFF,
+   IMX274_MODE_BINNING_2_1,
+   IMX274_MODE_BINNING_3_1,
 };
 
 /*
@@ -152,8 +168,8 @@ enum imx274_mode {
  * These are the values to configure the sensor in one of the
  * implemented modes.
  *
- * @size: recommended recording pixels
  * @init_regs: registers to initialize the mode
+ * @bin_ratio: downscale factor (e.g. 3 for 3:1 binning)
  * @min_frame_len: Minimum frame length for each mode (see "Frame Rate
  * Adjustment (CSI-2)" in the datasheet)
  * @min_SHR: Minimum SHR register value (see "Shutter Setti

[PATCH v4 2/8] media: imx274: consolidate per-mode data in imx274_frmfmt

[Luca Ceresoli]

Data about the implemented readout modes is partially stored in
imx274_formats[], the rest is scattered in several arrays. The latter
are then accessed using the mode index, e.g.:

  min_frame_len[priv->mode_index]

Consolidate all these data in imx274_formats[], and store a pointer to
the selected mode (i.e. imx274_formats[priv->mode_index]) in the main
device struct. This way code to use these data becomes more readable,
e.g.:

  priv->mode->min_frame_len

This removes lots of scaffolding code and keeps data about each mode
in a unique place.

Also remove a parameter to imx274_mode_regs() that is now unused.

While this adds the mode pointer to the device struct, it does not
remove the mode_index from it because mode_index is still used in two
dev_dbg() calls.  This will be handled in a follow-up commit.

Signed-off-by: Luca Ceresoli 
Cc: Sakari Ailus 

---
Changed v3 -> v4: nothing

Changed v2 -> v3: nothing

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 139 +
 1 file changed, 66 insertions(+), 73 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 8a8a11b8d75d..2ec31ae4e60d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -147,10 +147,28 @@ enum imx274_mode {
 };
 
 /*
- * imx274 format related structure
+ * Parameters for each imx274 readout mode.
+ *
+ * These are the values to configure the sensor in one of the
+ * implemented modes.
+ *
+ * @size: recommended recording pixels
+ * @init_regs: registers to initialize the mode
+ * @min_frame_len: Minimum frame length for each mode (see "Frame Rate
+ * Adjustment (CSI-2)" in the datasheet)
+ * @min_SHR: Minimum SHR register value (see "Shutter Setting (CSI-2)" in the
+ *   datasheet)
+ * @max_fps: Maximum frames per second
+ * @nocpiop: Number of clocks per internal offset period (see "Integration Time
+ *   in Each Readout Drive Mode (CSI-2)" in the datasheet)
  */
 struct imx274_frmfmt {
struct v4l2_frmsize_discrete size;
+   const struct reg_8 *init_regs;
+   int min_frame_len;
+   int min_SHR;
+   int max_fps;
+   int nocpiop;
 };
 
 /*
@@ -476,58 +494,35 @@ static const struct reg_8 imx274_tp_regs[] = {
{IMX274_TABLE_END, 0x00}
 };
 
-static const struct reg_8 *mode_table[] = {
-   [IMX274_MODE_3840X2160] = imx274_mode1_3840x2160_raw10,
-   [IMX274_MODE_1920X1080] = imx274_mode3_1920x1080_raw10,
-   [IMX274_MODE_1280X720]  = imx274_mode5_1280x720_raw10,
-};
-
-/*
- * imx274 format related structure
- */
+/* nocpiop happens to be the same number for the implemented modes */
 static const struct imx274_frmfmt imx274_formats[] = {
-   { {3840, 2160} },
-   { {1920, 1080} },
-   { {1280,  720} },
-};
-
-/*
- * minimal frame length for each mode
- * refer to datasheet section "Frame Rate Adjustment (CSI-2)"
- */
-static const int min_frame_len[] = {
-   4550, /* mode 1, 4K */
-   2310, /* mode 3, 1080p */
-   2310 /* mode 5, 720p */
-};
-
-/*
- * minimal numbers of SHR register
- * refer to datasheet table "Shutter Setting (CSI-2)"
- */
-static const int min_SHR[] = {
-   12, /* mode 1, 4K */
-   8, /* mode 3, 1080p */
-   8 /* mode 5, 720p */
-};
-
-static const int max_frame_rate[] = {
-   60, /* mode 1 , 4K */
-   120, /* mode 3, 1080p */
-   120 /* mode 5, 720p */
-};
-
-/*
- * Number of clocks per internal offset period
- * a constant based on mode
- * refer to section "Integration Time in Each Readout Drive Mode (CSI-2)"
- * in the datasheet
- * for the implemented 3 modes, it happens to be the same number
- */
-static const int nocpiop[] = {
-   112, /* mode 1 , 4K */
-   112, /* mode 3, 1080p */
-   112 /* mode 5, 720p */
+   {
+   /* mode 1, 4K */
+   .size = {3840, 2160},
+   .init_regs = imx274_mode1_3840x2160_raw10,
+   .min_frame_len = 4550,
+   .min_SHR = 12,
+   .max_fps = 60,
+   .nocpiop = 112,
+   },
+   {
+   /* mode 3, 1080p */
+   .size = {1920, 1080},
+   .init_regs = imx274_mode3_1920x1080_raw10,
+   .min_frame_len = 2310,
+   .min_SHR = 8,
+   .max_fps = 120,
+   .nocpiop = 112,
+   },
+   {
+   /* mode 5, 720p */
+   .size = {1280, 720},
+   .init_regs = imx274_mode5_1280x720_raw10,
+   .min_frame_len = 2310,
+   .min_SHR = 8,
+   .max_fps = 120,
+   .nocpiop = 112,
+   },
 };
 
 /*
@@ -557,6 +552,8 @@ struct imx274_ctrls {
  * @regmap: Pointer to regmap structure
  * @reset_gpio: Pointer to reset gpio
  * @lock: Mutex structure
+ * @mod

[PATCH v4 4/8] media: imx274: actually use IMX274_DEFAULT_MODE

[Luca Ceresoli]

IMX274_DEFAULT_MODE is defined but not used. Start using it, so the
default can be more easily changed without digging into the code.

Signed-off-by: Luca Ceresoli 
Cc: Sakari Ailus 

---
Changed v3 -> v4: nothing

Changed v2 -> v3: nothing

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index f075715ffced..ceeec97cd330 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1621,7 +1621,7 @@ static int imx274_probe(struct i2c_client *client,
mutex_init(>lock);
 
/* initialize format */
-   imx274->mode = _formats[IMX274_MODE_3840X2160];
+   imx274->mode = _formats[IMX274_DEFAULT_MODE];
imx274->format.width = imx274->mode->size.width;
imx274->format.height = imx274->mode->size.height;
imx274->format.field = V4L2_FIELD_NONE;
-- 
2.7.4

[PATCH v3 1/7] media: imx274: initialize format before v4l2 controls

[Luca Ceresoli]

The current probe function calls v4l2_ctrl_handler_setup() before
initializing the format info. This triggers call paths such as:
imx274_probe -> v4l2_ctrl_handler_setup -> imx274_s_ctrl ->
imx274_set_exposure, where priv->mode_index is accessed before being
assigned.

This is wrong but does not trigger a visible bug because priv is
zero-initialized and 0 is the default value for priv->mode_index. But
this would become a crash in follow-up commits when mode_index is
replaced by a pointer that must always be valid.

Fix the bug before it shows up by initializing struct members early.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ai...@linux.intel.com>

---
Changed v2 -> v3: nothing

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 63fb94e7da37..8a8a11b8d75d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1632,6 +1632,16 @@ static int imx274_probe(struct i2c_client *client,
 
mutex_init(>lock);
 
+   /* initialize format */
+   imx274->mode_index = IMX274_MODE_3840X2160;
+   imx274->format.width = imx274_formats[0].size.width;
+   imx274->format.height = imx274_formats[0].size.height;
+   imx274->format.field = V4L2_FIELD_NONE;
+   imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
+   imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
+   imx274->frame_interval.numerator = 1;
+   imx274->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
+
/* initialize regmap */
imx274->regmap = devm_regmap_init_i2c(client, _regmap_config);
if (IS_ERR(imx274->regmap)) {
@@ -1720,16 +1730,6 @@ static int imx274_probe(struct i2c_client *client,
goto err_ctrls;
}
 
-   /* initialize format */
-   imx274->mode_index = IMX274_MODE_3840X2160;
-   imx274->format.width = imx274_formats[0].size.width;
-   imx274->format.height = imx274_formats[0].size.height;
-   imx274->format.field = V4L2_FIELD_NONE;
-   imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
-   imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
-   imx274->frame_interval.numerator = 1;
-   imx274->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
-
/* load default control values */
ret = imx274_load_default(imx274);
if (ret) {
-- 
2.7.4

[PATCH v3 3/7] media: imx274: get rid of mode_index

[Luca Ceresoli]

After restructuring struct imx274_frmfmt, the mode_index field is
still in use only for two dev_dbg() calls in imx274_s_stream(). Let's
remove it and avoid duplicated information.

Replacing the first usage requires some rather annoying but trivial
pointer math. The other one can be removed entirely since it would
print the same value anyway.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ai...@linux.intel.com>

---
Changed v2 -> v3:
 - really fix dev_dbg() format mismatch warning for both 32 and 64 bit

Changed v1 -> v2:
 - add "media: " prefix to commit message
 - fix dev_dbg() format mismatch warning
   ("warning: format ‘%ld’ expects argument of type ‘long int’, but argument 6 
has type ‘int’")
 - slightly improve commit message
---
 drivers/media/i2c/imx274.c | 15 +--
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 2ec31ae4e60d..f075715ffced 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -553,8 +553,6 @@ struct imx274_ctrls {
  * @reset_gpio: Pointer to reset gpio
  * @lock: Mutex structure
  * @mode: Parameters for the selected readout mode
- *(points to imx274_formats[mode_index])
- * @mode_index: Resolution mode index
  */
 struct stimx274 {
struct v4l2_subdev sd;
@@ -567,7 +565,6 @@ struct stimx274 {
struct gpio_desc *reset_gpio;
struct mutex lock; /* mutex lock for operations */
const struct imx274_frmfmt *mode;
-   u32 mode_index;
 };
 
 /*
@@ -880,7 +877,6 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
index = 0;
}
 
-   imx274->mode_index = index;
imx274->mode = _formats[index];
 
if (fmt->width > IMX274_MAX_WIDTH)
@@ -1028,8 +1024,9 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
struct stimx274 *imx274 = to_imx274(sd);
int ret = 0;
 
-   dev_dbg(>client->dev, "%s : %s, mode index = %d\n", __func__,
-   on ? "Stream Start" : "Stream Stop", imx274->mode_index);
+   dev_dbg(>client->dev, "%s : %s, mode index = %td\n", __func__,
+   on ? "Stream Start" : "Stream Stop",
+   imx274->mode - _formats[0]);
 
mutex_lock(>lock);
 
@@ -1068,8 +1065,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
}
 
mutex_unlock(>lock);
-   dev_dbg(>client->dev,
-   "%s : Done: mode = %d\n", __func__, imx274->mode_index);
+   dev_dbg(>client->dev, "%s : Done\n", __func__);
return 0;
 
 fail:
@@ -1625,8 +1621,7 @@ static int imx274_probe(struct i2c_client *client,
mutex_init(>lock);
 
/* initialize format */
-   imx274->mode_index = IMX274_MODE_3840X2160;
-   imx274->mode = _formats[imx274->mode_index];
+   imx274->mode = _formats[IMX274_MODE_3840X2160];
imx274->format.width = imx274->mode->size.width;
imx274->format.height = imx274->mode->size.height;
imx274->format.field = V4L2_FIELD_NONE;
-- 
2.7.4

[PATCH v3 7/7] media: imx274: add SELECTION support for cropping

[Luca Ceresoli]

Currently this driver does not support cropping. The supported modes
are the following, all capturing the entire area:

 - 3840x2160, 1:1 binning (native sensor resolution)
 - 1920x1080, 2:1 binning
 - 1280x720,  3:1 binning

The set_fmt callback chooses among these 3 configurations the one that
matches the requested format.

Adding support to VIDIOC_SUBDEV_G_SELECTION and
VIDIOC_SUBDEV_S_SELECTION involved a complete rewrite of set_fmt,
which now chooses the most appropriate binning based on the ratio
between the previously-set cropping size and the format size being
requested.

Also change the names in enum imx274_mode from being resolution-based
to being binning-based (e.g. from IMX274_MODE_1920X1080 to
IMX274_MODE_BINNING_2_1). Without cropping, the two naming are
equivalent. With cropping, the resolution could be different,
e.g. using 2:1 binning mode to crop 1200x960 and output a 600x480
format. Using binning in the names avoids any misunderstanding.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ai...@linux.intel.com>

---
Changed v2 -> v3:
 - Remove hard-coded HMAX reg setting from all modes, not only the
   first one. HMAX is always computed and set in s_stream now.
 - Reword commit log message.

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 272 -
 1 file changed, 192 insertions(+), 80 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index e5ba19b97083..f5819ce99e98 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -19,6 +19,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -74,7 +75,7 @@
  */
 #define IMX274_MIN_EXPOSURE_TIME   (4 * 260 / 72)
 
-#define IMX274_DEFAULT_MODEIMX274_MODE_3840X2160
+#define IMX274_DEFAULT_MODEIMX274_MODE_BINNING_OFF
 #define IMX274_MAX_WIDTH   (3840)
 #define IMX274_MAX_HEIGHT  (2160)
 #define IMX274_MAX_FRAME_RATE  (120)
@@ -111,6 +112,20 @@
 #define IMX274_SHR_REG_LSB 0x300C /* SHR */
 #define IMX274_SVR_REG_MSB 0x300F /* SVR */
 #define IMX274_SVR_REG_LSB 0x300E /* SVR */
+#define IMX274_HTRIM_EN_REG0x3037
+#define IMX274_HTRIM_START_REG_LSB 0x3038
+#define IMX274_HTRIM_START_REG_MSB 0x3039
+#define IMX274_HTRIM_END_REG_LSB   0x303A
+#define IMX274_HTRIM_END_REG_MSB   0x303B
+#define IMX274_VWIDCUTEN_REG   0x30DD
+#define IMX274_VWIDCUT_REG_LSB 0x30DE
+#define IMX274_VWIDCUT_REG_MSB 0x30DF
+#define IMX274_VWINPOS_REG_LSB 0x30E0
+#define IMX274_VWINPOS_REG_MSB 0x30E1
+#define IMX274_WRITE_VSIZE_REG_LSB 0x3130
+#define IMX274_WRITE_VSIZE_REG_MSB 0x3131
+#define IMX274_Y_OUT_SIZE_REG_LSB  0x3132
+#define IMX274_Y_OUT_SIZE_REG_MSB  0x3133
 #define IMX274_VMAX_REG_1  0x30FA /* VMAX, MSB */
 #define IMX274_VMAX_REG_2  0x30F9 /* VMAX */
 #define IMX274_VMAX_REG_3  0x30F8 /* VMAX, LSB */
@@ -141,9 +156,9 @@ static const struct regmap_config imx274_regmap_config = {
 };
 
 enum imx274_mode {
-   IMX274_MODE_3840X2160,
-   IMX274_MODE_1920X1080,
-   IMX274_MODE_1280X720,
+   IMX274_MODE_BINNING_OFF,
+   IMX274_MODE_BINNING_2_1,
+   IMX274_MODE_BINNING_3_1,
 };
 
 /*
@@ -215,31 +230,14 @@ static const struct reg_8 imx274_mode1_3840x2160_raw10[] 
= {
{0x3004, 0x01},
{0x3005, 0x01},
{0x3006, 0x00},
-   {0x3007, 0x02},
+   {0x3007, 0xa2},
 
{0x3018, 0xA2}, /* output XVS, HVS */
 
{0x306B, 0x05},
{0x30E2, 0x01},
-   {0x30F6, 0x07}, /* HMAX, 263 */
-   {0x30F7, 0x01}, /* HMAX */
-
-   {0x30dd, 0x01}, /* crop to 2160 */
-   {0x30de, 0x06},
-   {0x30df, 0x00},
-   {0x30e0, 0x12},
-   {0x30e1, 0x00},
-   {0x3037, 0x01}, /* to crop to 3840 */
-   {0x3038, 0x0c},
-   {0x3039, 0x00},
-   {0x303a, 0x0c},
-   {0x303b, 0x0f},
 
{0x30EE, 0x01},
-   {0x3130, 0x86},
-   {0x3131, 0x08},
-   {0x3132, 0x7E},
-   {0x3133, 0x08},
{0x3342, 0x0A},
{0x3343, 0x00},
{0x3344, 0x16},
@@ -273,32 +271,14 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] 
= {
{0x3004, 0x02},
{0x3005, 0x21},
{0x3006, 0x00},
-   {0x3007, 0x11},
+   {0x3007, 0xb1},
 
{0x3018, 0xA2}, /* output XVS, HVS */
 
{0x306B, 0x05},
{0x30E2, 0x02},
 
-   {0x30F6, 0x04}, /* HMAX, 260 */
-   {0x30F7, 0x01}, /* HMAX */
-
-   {0x30dd, 0x01}, /* to crop to 1920x1080 */
-   {0x

[PATCH v3 4/7] media: imx274: actually use IMX274_DEFAULT_MODE

[Luca Ceresoli]

IMX274_DEFAULT_MODE is defined but not used. Start using it, so the
default can be more easily changed without digging into the code.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ai...@linux.intel.com>

---
Changed v2 -> v3: nothing

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index f075715ffced..ceeec97cd330 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1621,7 +1621,7 @@ static int imx274_probe(struct i2c_client *client,
mutex_init(>lock);
 
/* initialize format */
-   imx274->mode = _formats[IMX274_MODE_3840X2160];
+   imx274->mode = _formats[IMX274_DEFAULT_MODE];
imx274->format.width = imx274->mode->size.width;
imx274->format.height = imx274->mode->size.height;
imx274->format.field = V4L2_FIELD_NONE;
-- 
2.7.4

[PATCH v3 5/7] media: imx274: simplify imx274_write_table()

[Luca Ceresoli]

imx274_write_table() is a mere wrapper (and the only user) to
imx274_regmap_util_write_table_8(). Remove this useless indirection by
merging the two functions into one.

Also get rid of the wait_ms_addr and end_addr parameters since it does
not make any sense to give them any values other than
IMX274_TABLE_WAIT_MS and IMX274_TABLE_END.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ai...@linux.intel.com>

---
Changed v2 -> v3: nothing

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 28 ++--
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index ceeec97cd330..48343c2ade83 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -597,20 +597,18 @@ static inline struct stimx274 *to_imx274(struct 
v4l2_subdev *sd)
 }
 
 /*
- * imx274_regmap_util_write_table_8 - Function for writing register table
- * @regmap: Pointer to device reg map structure
- * @table: Table containing register values
- * @wait_ms_addr: Flag for performing delay
- * @end_addr: Flag for incating end of table
+ * Writing a register table
+ *
+ * @priv: Pointer to device
+ * @table: Table containing register values (with optional delays)
  *
  * This is used to write register table into sensor's reg map.
  *
  * Return: 0 on success, errors otherwise
  */
-static int imx274_regmap_util_write_table_8(struct regmap *regmap,
-   const struct reg_8 table[],
-   u16 wait_ms_addr, u16 end_addr)
+static int imx274_write_table(struct stimx274 *priv, const struct reg_8 
table[])
 {
+   struct regmap *regmap = priv->regmap;
int err = 0;
const struct reg_8 *next;
u8 val;
@@ -622,8 +620,8 @@ static int imx274_regmap_util_write_table_8(struct regmap 
*regmap,
 
for (next = table;; next++) {
if ((next->addr != range_start + range_count) ||
-   (next->addr == end_addr) ||
-   (next->addr == wait_ms_addr) ||
+   (next->addr == IMX274_TABLE_END) ||
+   (next->addr == IMX274_TABLE_WAIT_MS) ||
(range_count == max_range_vals)) {
if (range_count == 1)
err = regmap_write(regmap,
@@ -642,10 +640,10 @@ static int imx274_regmap_util_write_table_8(struct regmap 
*regmap,
range_count = 0;
 
/* Handle special address values */
-   if (next->addr == end_addr)
+   if (next->addr == IMX274_TABLE_END)
break;
 
-   if (next->addr == wait_ms_addr) {
+   if (next->addr == IMX274_TABLE_WAIT_MS) {
msleep_range(next->val);
continue;
}
@@ -692,12 +690,6 @@ static inline int imx274_write_reg(struct stimx274 *priv, 
u16 addr, u8 val)
return err;
 }
 
-static int imx274_write_table(struct stimx274 *priv, const struct reg_8 
table[])
-{
-   return imx274_regmap_util_write_table_8(priv->regmap,
-   table, IMX274_TABLE_WAIT_MS, IMX274_TABLE_END);
-}
-
 /*
  * Set mode registers to start stream.
  * @priv: Pointer to device structure
-- 
2.7.4

[PATCH v3 0/7] media: imx274: cleanups, improvements and SELECTION API support

[Luca Ceresoli]

Hi,

this patchset introduces cropping support for the Sony IMX274 sensor
using the SELECTION API. 

v3 has a few minor improvements over v2. It also removes the first 6
patches, already applied on the media_tree master branch.

After v2 there has been a short discussion with Sakari Ailus on how
cropping should be configured from userspace [0]. That discussion has
gone stale before I could understand the idea behind the changes
suggested by Sakari, so I'm sending the most up-to-date version of the
old implementation to give it a new spin. I'll be glad to rework my
patch when things are clearer.

Patches 1-5 are overall improvements and restructuring, mostly useful
to implement the SELECTION API in a clean way.

Patch 6 introduces a helper to allow setting many registers computed
at runtime in a straightforward way. This would not have been very
useful before because all long register write sequences came from
const tables, but it's definitely a must for the cropping code where
several register values are computed at runtime.

Patch 7 implements cropping in the set_selection pad operation. On the
v4l2 side there is nothing special. The most tricky part was
respecting all the device constraints on the horizontal crop.

Usage examples:

 * Capture the entire 4K area, downscaled to 1080p with 2:1 binning:
   media-ctl -V '"IMX274":0[crop:(0,0)/3840x2160]'
   media-ctl -V '"IMX274":0[fmt:SRGGB8_1X8/1920x1080 field:none]'

 * Capture the central 1080p area (no binning):
   media-ctl -V '"IMX274":0[crop:(960,540)/1920x1080]'
   (this also sets the fmt to 1920x1080)  

Regards,
Luca

[0] https://www.spinics.net/lists/kernel/msg2787725.html


Luca Ceresoli (7):
  media: imx274: initialize format before v4l2 controls
  media: imx274: consolidate per-mode data in imx274_frmfmt
  media: imx274: get rid of mode_index
  media: imx274: actually use IMX274_DEFAULT_MODE
  media: imx274: simplify imx274_write_table()
  media: imx274: add helper function to fill a reg_8 table chunk
  media: imx274: add SELECTION support for cropping

 drivers/media/i2c/imx274.c | 552 +++--
 1 file changed, 332 insertions(+), 220 deletions(-)

-- 
2.7.4

[PATCH v3 2/7] media: imx274: consolidate per-mode data in imx274_frmfmt

[Luca Ceresoli]

Data about the implemented readout modes is partially stored in
imx274_formats[], the rest is scattered in several arrays. The latter
are then accessed using the mode index, e.g.:

  min_frame_len[priv->mode_index]

Consolidate all these data in imx274_formats[], and store a pointer to
the selected mode (i.e. imx274_formats[priv->mode_index]) in the main
device struct. This way code to use these data becomes more readable,
e.g.:

  priv->mode->min_frame_len

This removes lots of scaffolding code and keeps data about each mode
in a unique place.

Also remove a parameter to imx274_mode_regs() that is now unused.

While this adds the mode pointer to the device struct, it does not
remove the mode_index from it because mode_index is still used in two
dev_dbg() calls.  This will be handled in a follow-up commit.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ai...@linux.intel.com>

---
Changed v2 -> v3: nothing

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 139 +
 1 file changed, 66 insertions(+), 73 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 8a8a11b8d75d..2ec31ae4e60d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -147,10 +147,28 @@ enum imx274_mode {
 };
 
 /*
- * imx274 format related structure
+ * Parameters for each imx274 readout mode.
+ *
+ * These are the values to configure the sensor in one of the
+ * implemented modes.
+ *
+ * @size: recommended recording pixels
+ * @init_regs: registers to initialize the mode
+ * @min_frame_len: Minimum frame length for each mode (see "Frame Rate
+ * Adjustment (CSI-2)" in the datasheet)
+ * @min_SHR: Minimum SHR register value (see "Shutter Setting (CSI-2)" in the
+ *   datasheet)
+ * @max_fps: Maximum frames per second
+ * @nocpiop: Number of clocks per internal offset period (see "Integration Time
+ *   in Each Readout Drive Mode (CSI-2)" in the datasheet)
  */
 struct imx274_frmfmt {
struct v4l2_frmsize_discrete size;
+   const struct reg_8 *init_regs;
+   int min_frame_len;
+   int min_SHR;
+   int max_fps;
+   int nocpiop;
 };
 
 /*
@@ -476,58 +494,35 @@ static const struct reg_8 imx274_tp_regs[] = {
{IMX274_TABLE_END, 0x00}
 };
 
-static const struct reg_8 *mode_table[] = {
-   [IMX274_MODE_3840X2160] = imx274_mode1_3840x2160_raw10,
-   [IMX274_MODE_1920X1080] = imx274_mode3_1920x1080_raw10,
-   [IMX274_MODE_1280X720]  = imx274_mode5_1280x720_raw10,
-};
-
-/*
- * imx274 format related structure
- */
+/* nocpiop happens to be the same number for the implemented modes */
 static const struct imx274_frmfmt imx274_formats[] = {
-   { {3840, 2160} },
-   { {1920, 1080} },
-   { {1280,  720} },
-};
-
-/*
- * minimal frame length for each mode
- * refer to datasheet section "Frame Rate Adjustment (CSI-2)"
- */
-static const int min_frame_len[] = {
-   4550, /* mode 1, 4K */
-   2310, /* mode 3, 1080p */
-   2310 /* mode 5, 720p */
-};
-
-/*
- * minimal numbers of SHR register
- * refer to datasheet table "Shutter Setting (CSI-2)"
- */
-static const int min_SHR[] = {
-   12, /* mode 1, 4K */
-   8, /* mode 3, 1080p */
-   8 /* mode 5, 720p */
-};
-
-static const int max_frame_rate[] = {
-   60, /* mode 1 , 4K */
-   120, /* mode 3, 1080p */
-   120 /* mode 5, 720p */
-};
-
-/*
- * Number of clocks per internal offset period
- * a constant based on mode
- * refer to section "Integration Time in Each Readout Drive Mode (CSI-2)"
- * in the datasheet
- * for the implemented 3 modes, it happens to be the same number
- */
-static const int nocpiop[] = {
-   112, /* mode 1 , 4K */
-   112, /* mode 3, 1080p */
-   112 /* mode 5, 720p */
+   {
+   /* mode 1, 4K */
+   .size = {3840, 2160},
+   .init_regs = imx274_mode1_3840x2160_raw10,
+   .min_frame_len = 4550,
+   .min_SHR = 12,
+   .max_fps = 60,
+   .nocpiop = 112,
+   },
+   {
+   /* mode 3, 1080p */
+   .size = {1920, 1080},
+   .init_regs = imx274_mode3_1920x1080_raw10,
+   .min_frame_len = 2310,
+   .min_SHR = 8,
+   .max_fps = 120,
+   .nocpiop = 112,
+   },
+   {
+   /* mode 5, 720p */
+   .size = {1280, 720},
+   .init_regs = imx274_mode5_1280x720_raw10,
+   .min_frame_len = 2310,
+   .min_SHR = 8,
+   .max_fps = 120,
+   .nocpiop = 112,
+   },
 };
 
 /*
@@ -557,6 +552,8 @@ struct imx274_ctrls {
  * @regmap: Pointer to regmap structure
  * @reset_gpio: Pointer to reset gpio
  * @lock: Mute

[PATCH v3 6/7] media: imx274: add helper function to fill a reg_8 table chunk

[Luca Ceresoli]

Tables of struct reg_8 are used to simplify multi-byte register
assignment. However filling these snippets with values computed at
runtime is currently implemented by very similar functions doing the
needed shift & mask manipulation.

Replace all those functions with a unique helper function to fill
reg_8 tables in a simple and clean way.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
Cc: Sakari Ailus <sakari.ai...@linux.intel.com>

---
Changed v2 -> v3:
 - minor reformatting in prepare_reg() documentation

Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 90 --
 1 file changed, 55 insertions(+), 35 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 48343c2ade83..e5ba19b97083 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -597,6 +597,58 @@ static inline struct stimx274 *to_imx274(struct 
v4l2_subdev *sd)
 }
 
 /*
+ * Fill consecutive reg_8 items in order to set a multibyte register.
+ *
+ * The sensor has many 2-bytes registers and a 3-byte register. This
+ * simplifies code to set them by filling consecutive entries of a
+ * struct reg_8 table with the data to set a register.
+ *
+ * The number of table entries that is filled is the minimum needed
+ * for the given number of bits (i.e. nbits / 8, rounded up). If nbits
+ * is not a multiple of 8, extra bits in the most significant byte are
+ * zeroed.
+ *
+ * Example:
+ *   Calling prepare_reg([10], 0x3000, 0xcafe, 12) will set:
+ *   regs[10] = { .addr = 0x3000, .val = 0xfe }
+ *   regs[11] = { .addr = 0x3001, .val = 0x0a }
+ *
+ * @table_base: Pointer to the first reg_8 struct to be filled.  The
+ *  following entries will also be set, make sure they are
+ *  properly allocated!
+ * @addr_lsb: Address of the LSB register.  Other registers must be
+ *consecutive, least-to-most significant.
+ * @value: Value to be written to the register.
+ * @nbits: Number of bits to write (range: [1..24])
+ */
+static void prepare_reg(struct reg_8 *table_base,
+   u16 addr_lsb,
+   u32 value,
+   size_t nbits)
+{
+   struct reg_8 *cmd = table_base;
+   u16 addr = addr_lsb;
+   size_t bits; /* how many bits at this round */
+
+   WARN_ON(nbits > 24);
+
+   if (nbits > 24)
+   nbits = 24;
+
+   while (nbits > 0) {
+   bits = min_t(size_t, 8, nbits);
+
+   cmd->addr = addr;
+   cmd->val = value & ((1 << bits) - 1);
+
+   nbits -= bits;
+   value >>= 8;
+   addr++;
+   cmd++;
+   }
+}
+
+/*
  * Writing a register table
  *
  * @priv: Pointer to device
@@ -1163,15 +1215,6 @@ static int imx274_set_digital_gain(struct stimx274 
*priv, u32 dgain)
reg_val & IMX274_MASK_LSB_4_BITS);
 }
 
-static inline void imx274_calculate_gain_regs(struct reg_8 regs[2], u16 gain)
-{
-   regs->addr = IMX274_ANALOG_GAIN_ADDR_MSB;
-   regs->val = (gain >> IMX274_SHIFT_8_BITS) & IMX274_MASK_LSB_3_BITS;
-
-   (regs + 1)->addr = IMX274_ANALOG_GAIN_ADDR_LSB;
-   (regs + 1)->val = (gain) & IMX274_MASK_LSB_8_BITS;
-}
-
 /*
  * imx274_set_gain - Function called when setting gain
  * @priv: Pointer to device structure
@@ -1229,7 +1272,7 @@ static int imx274_set_gain(struct stimx274 *priv, struct 
v4l2_ctrl *ctrl)
if (gain_reg > IMX274_GAIN_REG_MAX)
gain_reg = IMX274_GAIN_REG_MAX;
 
-   imx274_calculate_gain_regs(reg_list, (u16)gain_reg);
+   prepare_reg(reg_list, IMX274_ANALOG_GAIN_ADDR_LSB, gain_reg, 11);
 
for (i = 0; i < ARRAY_SIZE(reg_list); i++) {
err = imx274_write_reg(priv, reg_list[i].addr,
@@ -1258,16 +1301,6 @@ static int imx274_set_gain(struct stimx274 *priv, struct 
v4l2_ctrl *ctrl)
return err;
 }
 
-static inline void imx274_calculate_coarse_time_regs(struct reg_8 regs[2],
-u32 coarse_time)
-{
-   regs->addr = IMX274_SHR_REG_MSB;
-   regs->val = (coarse_time >> IMX274_SHIFT_8_BITS)
-   & IMX274_MASK_LSB_8_BITS;
-   (regs + 1)->addr = IMX274_SHR_REG_LSB;
-   (regs + 1)->val = (coarse_time) & IMX274_MASK_LSB_8_BITS;
-}
-
 /*
  * imx274_set_coarse_time - Function called when setting SHR value
  * @priv: Pointer to device structure
@@ -1292,7 +1325,7 @@ static int imx274_set_coarse_time(struct stimx274 *priv, 
u32 *val)
goto fail;
 
/* prepare SHR registers */
-   imx274_calculate_coarse_time_regs(reg_list, coarse_time);
+   prepare_reg(reg_list, IMX274_SHR_REG_LSB, coarse_time, 16);
 
/* write to SHR registers */
for (i = 0; i < ARRAY_SIZE

[PATCH v2 2/5] media: docs: clarify relationship between crop and selection APIs

[Luca Ceresoli]

Having two somewhat similar and largely overlapping APIs is confusing,
especially since the older one appears in the docs before the newer
and most featureful counterpart.

Clarify all of this in several ways:
 - swap the two sections
 - give a name to the two APIs in the section names
 - add a note at the beginning of the CROP API section
 - update note about VIDIOC_CROPCAP

Also remove a note that is incorrect (correct wording is in
vidioc-cropcap.rst).

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
Based on info from: Hans Verkuil <hverk...@xs4all.nl>
Cc: Hans Verkuil <hverk...@xs4all.nl>

---

Changed v1 -> v2:
 - Don't remove the note about VIDIOC_CROPCAP being mandatory, update
   it (Hans)
---
 Documentation/media/uapi/v4l/common.rst|  2 +-
 Documentation/media/uapi/v4l/crop.rst  | 22 +++---
 Documentation/media/uapi/v4l/selection-api-005.rst |  2 ++
 Documentation/media/uapi/v4l/selection-api.rst |  4 ++--
 4 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/Documentation/media/uapi/v4l/common.rst 
b/Documentation/media/uapi/v4l/common.rst
index 13f2ed3fc5a6..5f93e71122ef 100644
--- a/Documentation/media/uapi/v4l/common.rst
+++ b/Documentation/media/uapi/v4l/common.rst
@@ -41,6 +41,6 @@ applicable to all devices.
 extended-controls
 format
 planar-apis
-crop
 selection-api
+crop
 streaming-par
diff --git a/Documentation/media/uapi/v4l/crop.rst 
b/Documentation/media/uapi/v4l/crop.rst
index 182565b9ace4..45e8a895a320 100644
--- a/Documentation/media/uapi/v4l/crop.rst
+++ b/Documentation/media/uapi/v4l/crop.rst
@@ -2,9 +2,18 @@
 
 .. _crop:
 
-*
-Image Cropping, Insertion and Scaling
-*
+*
+Image Cropping, Insertion and Scaling -- the CROP API
+*
+
+.. note::
+
+   The CROP API is mostly superseded by the newer :ref:`SELECTION API
+   `. The new API should be preferred in most cases,
+   with the exception of pixel aspect ratio detection, which is
+   implemented by :ref:`VIDIOC_CROPCAP ` and has no
+   equivalent in the SELECTION API. See :ref:`selection-vs-crop` for a
+   comparison of the two APIs.
 
 Some video capture devices can sample a subsection of the picture and
 shrink or enlarge it to an image of arbitrary size. We call these
@@ -42,10 +51,9 @@ where applicable) will be fixed in this case.
 
 .. note::
 
-   All capture and output devices must support the
-   :ref:`VIDIOC_CROPCAP ` ioctl such that applications
-   can determine if scaling takes place.
-
+   All capture and output devices that support the CROP or SELECTION
+   API will also support the :ref:`VIDIOC_CROPCAP `
+   ioctl.
 
 Cropping Structures
 ===
diff --git a/Documentation/media/uapi/v4l/selection-api-005.rst 
b/Documentation/media/uapi/v4l/selection-api-005.rst
index 5b47a28ac6d7..2ad30a49184f 100644
--- a/Documentation/media/uapi/v4l/selection-api-005.rst
+++ b/Documentation/media/uapi/v4l/selection-api-005.rst
@@ -1,5 +1,7 @@
 .. -*- coding: utf-8; mode: rst -*-
 
+.. _selection-vs-crop:
+
 
 Comparison with old cropping API
 
diff --git a/Documentation/media/uapi/v4l/selection-api.rst 
b/Documentation/media/uapi/v4l/selection-api.rst
index 81ea52d785b9..e4e623824b30 100644
--- a/Documentation/media/uapi/v4l/selection-api.rst
+++ b/Documentation/media/uapi/v4l/selection-api.rst
@@ -2,8 +2,8 @@
 
 .. _selection-api:
 
-API for cropping, composing and scaling
-===
+Cropping, composing and scaling -- the SELECTION API
+
 
 
 .. toctree::
-- 
2.7.4

[PATCH v2 3/5] media: docs: selection: rename files to something meaningful

[Luca Ceresoli]

These files have an automatically-generated numbering. Rename them
with a name that suggests their meaning.

Reported-by: Hans Verkuil <hverk...@xs4all.nl>
Cc: Hans Verkuil <hverk...@xs4all.nl>
Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>

---

Changed v1 -> v2:
 - fix commit message typos (Hans)
---
 .../{selection-api-004.rst => selection-api-configuration.rst} |  0
 .../v4l/{selection-api-006.rst => selection-api-examples.rst}  |  0
 .../v4l/{selection-api-002.rst => selection-api-intro.rst} |  0
 .../v4l/{selection-api-003.rst => selection-api-targets.rst}   |  0
 .../{selection-api-005.rst => selection-api-vs-crop-api.rst}   |  0
 Documentation/media/uapi/v4l/selection-api.rst | 10 +-
 6 files changed, 5 insertions(+), 5 deletions(-)
 rename Documentation/media/uapi/v4l/{selection-api-004.rst => 
selection-api-configuration.rst} (100%)
 rename Documentation/media/uapi/v4l/{selection-api-006.rst => 
selection-api-examples.rst} (100%)
 rename Documentation/media/uapi/v4l/{selection-api-002.rst => 
selection-api-intro.rst} (100%)
 rename Documentation/media/uapi/v4l/{selection-api-003.rst => 
selection-api-targets.rst} (100%)
 rename Documentation/media/uapi/v4l/{selection-api-005.rst => 
selection-api-vs-crop-api.rst} (100%)

diff --git a/Documentation/media/uapi/v4l/selection-api-004.rst 
b/Documentation/media/uapi/v4l/selection-api-configuration.rst
similarity index 100%
rename from Documentation/media/uapi/v4l/selection-api-004.rst
rename to Documentation/media/uapi/v4l/selection-api-configuration.rst
diff --git a/Documentation/media/uapi/v4l/selection-api-006.rst 
b/Documentation/media/uapi/v4l/selection-api-examples.rst
similarity index 100%
rename from Documentation/media/uapi/v4l/selection-api-006.rst
rename to Documentation/media/uapi/v4l/selection-api-examples.rst
diff --git a/Documentation/media/uapi/v4l/selection-api-002.rst 
b/Documentation/media/uapi/v4l/selection-api-intro.rst
similarity index 100%
rename from Documentation/media/uapi/v4l/selection-api-002.rst
rename to Documentation/media/uapi/v4l/selection-api-intro.rst
diff --git a/Documentation/media/uapi/v4l/selection-api-003.rst 
b/Documentation/media/uapi/v4l/selection-api-targets.rst
similarity index 100%
rename from Documentation/media/uapi/v4l/selection-api-003.rst
rename to Documentation/media/uapi/v4l/selection-api-targets.rst
diff --git a/Documentation/media/uapi/v4l/selection-api-005.rst 
b/Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst
similarity index 100%
rename from Documentation/media/uapi/v4l/selection-api-005.rst
rename to Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst
diff --git a/Documentation/media/uapi/v4l/selection-api.rst 
b/Documentation/media/uapi/v4l/selection-api.rst
index e4e623824b30..390233f704a3 100644
--- a/Documentation/media/uapi/v4l/selection-api.rst
+++ b/Documentation/media/uapi/v4l/selection-api.rst
@@ -9,8 +9,8 @@ Cropping, composing and scaling -- the SELECTION API
 .. toctree::
 :maxdepth: 1
 
-selection-api-002
-selection-api-003
-selection-api-004
-selection-api-005
-selection-api-006
+selection-api-intro.rst
+selection-api-targets.rst
+selection-api-configuration.rst
+selection-api-vs-crop-api.rst
+selection-api-examples.rst
-- 
2.7.4

[PATCH v2 5/5] media: docs: selection: fix misleading sentence about the CROP API

[Luca Ceresoli]

The API limitation described here is about the CROP API, not about the
entire V4L2.

Cc: Hans Verkuil <hverk...@xs4all.nl>
Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>

---

Changed v1 -> v2: nothing.
---
 Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst 
b/Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst
index ba1064a244a0..e7455fb1e572 100644
--- a/Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst
+++ b/Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst
@@ -15,7 +15,7 @@ because the described operation is actually the composing. The
 selection API makes a clear distinction between composing and cropping
 operations by setting the appropriate targets.
 
-The V4L2 API lacks any support for composing to and cropping from an
+The CROP API lacks any support for composing to and cropping from an
 image inside a memory buffer. The application could configure a
 capture device to fill only a part of an image by abusing V4L2
 API. Cropping a smaller image from a larger one is achieved by setting
-- 
2.7.4

[PATCH v2 1/5] media: docs: selection: fix typos

[Luca Ceresoli]

Fix typos in the selection documentation:
 - over -> cover
 - BONDS -> BOUNDS

Cc: Hans Verkuil <hverk...@xs4all.nl>
Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>

---

Changed v1 -> v2:
 - add a commit message (Hans)
---
 Documentation/media/uapi/v4l/selection-api-004.rst | 2 +-
 Documentation/media/uapi/v4l/selection.svg | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/media/uapi/v4l/selection-api-004.rst 
b/Documentation/media/uapi/v4l/selection-api-004.rst
index d782cd5b2117..0a4ddc2d71db 100644
--- a/Documentation/media/uapi/v4l/selection-api-004.rst
+++ b/Documentation/media/uapi/v4l/selection-api-004.rst
@@ -41,7 +41,7 @@ The driver may further adjust the requested size and/or 
position
 according to hardware limitations.
 
 Each capture device has a default source rectangle, given by the
-``V4L2_SEL_TGT_CROP_DEFAULT`` target. This rectangle shall over what the
+``V4L2_SEL_TGT_CROP_DEFAULT`` target. This rectangle shall cover what the
 driver writer considers the complete picture. Drivers shall set the
 active crop rectangle to the default when the driver is first loaded,
 but not later.
diff --git a/Documentation/media/uapi/v4l/selection.svg 
b/Documentation/media/uapi/v4l/selection.svg
index a93e3b59786d..911062bd2844 100644
--- a/Documentation/media/uapi/v4l/selection.svg
+++ b/Documentation/media/uapi/v4l/selection.svg
@@ -1128,11 +1128,11 @@

   
   
-   COMPOSE_BONDS
+   COMPOSE_BOUNDS
   
   

-CROP_BONDS
+CROP_BOUNDS


 overscan area
-- 
2.7.4

[PATCH v2 4/5] media: docs: selection: improve formatting

[Luca Ceresoli]

Split section "Comparison with old cropping API" in paragraphs for
easier reading and improve visible links text.

Cc: Hans Verkuil <hverk...@xs4all.nl>
Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>

---

Changed v1 -> v2: nothing.
---
 .../media/uapi/v4l/selection-api-vs-crop-api.rst   | 55 --
 1 file changed, 29 insertions(+), 26 deletions(-)

diff --git a/Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst 
b/Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst
index 2ad30a49184f..ba1064a244a0 100644
--- a/Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst
+++ b/Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst
@@ -6,31 +6,34 @@
 Comparison with old cropping API
 
 
-The selection API was introduced to cope with deficiencies of previous
-:ref:`API `, that was designed to control simple capture
-devices. Later the cropping API was adopted by video output drivers. The
-ioctls are used to select a part of the display were the video signal is
-inserted. It should be considered as an API abuse because the described
-operation is actually the composing. The selection API makes a clear
-distinction between composing and cropping operations by setting the
-appropriate targets. The V4L2 API lacks any support for composing to and
-cropping from an image inside a memory buffer. The application could
-configure a capture device to fill only a part of an image by abusing
-V4L2 API. Cropping a smaller image from a larger one is achieved by
-setting the field ``bytesperline`` at struct
-:c:type:`v4l2_pix_format`.
-Introducing an image offsets could be done by modifying field ``m_userptr``
-at struct
-:c:type:`v4l2_buffer` before calling
-:ref:`VIDIOC_QBUF`. Those operations should be avoided because they are not
-portable (endianness), and do not work for macroblock and Bayer formats
-and mmap buffers. The selection API deals with configuration of buffer
+The selection API was introduced to cope with deficiencies of the
+older :ref:`CROP API `, that was designed to control simple
+capture devices. Later the cropping API was adopted by video output
+drivers. The ioctls are used to select a part of the display were the
+video signal is inserted. It should be considered as an API abuse
+because the described operation is actually the composing. The
+selection API makes a clear distinction between composing and cropping
+operations by setting the appropriate targets.
+
+The V4L2 API lacks any support for composing to and cropping from an
+image inside a memory buffer. The application could configure a
+capture device to fill only a part of an image by abusing V4L2
+API. Cropping a smaller image from a larger one is achieved by setting
+the field ``bytesperline`` at struct :c:type:`v4l2_pix_format`.
+Introducing an image offsets could be done by modifying field
+``m_userptr`` at struct :c:type:`v4l2_buffer` before calling
+:ref:`VIDIOC_QBUF `. Those operations should be avoided
+because they are not portable (endianness), and do not work for
+macroblock and Bayer formats and mmap buffers.
+
+The selection API deals with configuration of buffer
 cropping/composing in a clear, intuitive and portable way. Next, with
 the selection API the concepts of the padded target and constraints
-flags are introduced. Finally, struct :c:type:`v4l2_crop`
-and struct :c:type:`v4l2_cropcap` have no reserved
-fields. Therefore there is no way to extend their functionality. The new
-struct :c:type:`v4l2_selection` provides a lot of place
-for future extensions. Driver developers are encouraged to implement
-only selection API. The former cropping API would be simulated using the
-new one.
+flags are introduced. Finally, struct :c:type:`v4l2_crop` and struct
+:c:type:`v4l2_cropcap` have no reserved fields. Therefore there is no
+way to extend their functionality. The new struct
+:c:type:`v4l2_selection` provides a lot of place for future
+extensions.
+
+Driver developers are encouraged to implement only selection API. The
+former cropping API would be simulated using the new one.
-- 
2.7.4

Re: [PATCH 1/5] media: docs: selection: fix typos

[Luca Ceresoli]

Hi Hans,

On 13/05/2018 11:19, Hans Verkuil wrote:
> Hi Luca,
> 
> My apologies for the long delay in reviewing this.
> 
> It all looks very good and if you can post a v2 with these small issues
> fixed, then I'll merge it for 4.18.

On its way!

Thanks,
-- 
Luca

Re: [PATCH 2/5] media: docs: clarify relationship between crop and selection APIs

[Luca Ceresoli]

Hi Hans,

thanks for the review.

On 13/05/2018 11:12, Hans Verkuil wrote:
> On 04/03/2018 11:15 PM, Luca Ceresoli wrote:
>> Having two somewhat similar and largely overlapping APIs is confusing,
>> especially since the older one appears in the docs before the newer
>> and most featureful counterpart.
>>
>> Clarify all of this in several ways:
>>  - swap the two sections
>>  - give a name to the two APIs in the section names
>>  - add a note at the beginning of the CROP API section
>>
>> Also remove a note that is incorrect (correct wording is in
>> vidioc-cropcap.rst).
>>
>> Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
>> Based on info from: Hans Verkuil <hverk...@xs4all.nl>
>> Cc: Hans Verkuil <hverk...@xs4all.nl>
>> ---
>>  Documentation/media/uapi/v4l/common.rst|  2 +-
>>  Documentation/media/uapi/v4l/crop.rst  | 21 
>> -
>>  Documentation/media/uapi/v4l/selection-api-005.rst |  2 ++
>>  Documentation/media/uapi/v4l/selection-api.rst |  4 ++--
>>  4 files changed, 17 insertions(+), 12 deletions(-)
>>
>> diff --git a/Documentation/media/uapi/v4l/common.rst 
>> b/Documentation/media/uapi/v4l/common.rst
>> index 13f2ed3fc5a6..5f93e71122ef 100644
>> --- a/Documentation/media/uapi/v4l/common.rst
>> +++ b/Documentation/media/uapi/v4l/common.rst
>> @@ -41,6 +41,6 @@ applicable to all devices.
>>  extended-controls
>>  format
>>  planar-apis
>> -crop
>>  selection-api
>> +crop
>>  streaming-par
>> diff --git a/Documentation/media/uapi/v4l/crop.rst 
>> b/Documentation/media/uapi/v4l/crop.rst
>> index 182565b9ace4..83fa16eb347e 100644
>> --- a/Documentation/media/uapi/v4l/crop.rst
>> +++ b/Documentation/media/uapi/v4l/crop.rst
>> @@ -2,9 +2,18 @@
>>  
>>  .. _crop:
>>  
>> -*
>> -Image Cropping, Insertion and Scaling
>> -*
>> +*
>> +Image Cropping, Insertion and Scaling -- the CROP API
>> +*
>> +
>> +.. note::
>> +
>> +   The CROP API is mostly superseded by the newer :ref:`SELECTION API
>> +   `. The new API should be preferred in most cases,
>> +   with the exception of pixel aspect ratio detection, which is
>> +   implemented by :ref:`VIDIOC_CROPCAP ` and has no
>> +   equivalent in the SELECTION API. See :ref:`selection-vs-crop` for a
>> +   comparison of the two APIs.
>>  
>>  Some video capture devices can sample a subsection of the picture and
>>  shrink or enlarge it to an image of arbitrary size. We call these
>> @@ -40,12 +49,6 @@ support scaling or the :ref:`VIDIOC_G_CROP 
>> ` and
>>  :ref:`VIDIOC_S_CROP ` ioctls. Their size (and position
>>  where applicable) will be fixed in this case.
>>  
>> -.. note::
>> -
>> -   All capture and output devices must support the
>> -   :ref:`VIDIOC_CROPCAP ` ioctl such that applications
>> -   can determine if scaling takes place.
> 
> This note should be rewritten, not deleted:
> 
>   All capture and output devices that support the CROP or SELECTION API
>   will also support the :ref:`VIDIOC_CROPCAP ` ioctl.

I don't remember exactly the reationale for this removal, perhaps it's
that I added a note above with similar content. But reading that again
now I realize the added not does not clearly state "VIDIOC_CROPCAP is
mandatory".

Thus I added the updated note to v2 as you suggested.

Bye,
-- 
Luca

Re: [PATCH v2 13/13] media: imx274: add SELECTION support for cropping

[Luca Ceresoli]

Hi Sakari,

thanks for your feedback, see below my replies.

On 26/04/2018 10:13, Sakari Ailus wrote:
> Hi Luca,
> 
> On Tue, Apr 24, 2018 at 04:30:38PM +0200, Luca Ceresoli wrote:
>> Hi Sakari,
>>
>> On 24/04/2018 11:59, Sakari Ailus wrote:
>>> Hi Luca,
>>>
>>> Thank you for the patchset.
>>>
>>> Some comments below... what I propose is that I apply the rest of the
>>> patches and then the comments to this one could be addressed separately.
>>> Would that work for you?
>>
>> Yes, but I suggest you only apply patches 1-6. I noticed a warning in
>> patch 9, and patches 7-8 are not very useful without it. Will fix it in v3.
> 
> Ack. I'll apply 1--6 then.

Thanks.

>> I'll wait for the outcome of the discussion below before sending v3.
>> Tell me if you prefer me to do it earlier.
>>
>>> On Tue, Apr 24, 2018 at 10:24:18AM +0200, Luca Ceresoli wrote:
>>>> Currently this driver does not support cropping. The supported modes
>>>> are the following, all capturing the entire area:
>>>>
>>>>  - 3840x2160, 1:1 binning (native sensor resolution)
>>>>  - 1920x1080, 2:1 binning
>>>>  - 1280x720,  3:1 binning
>>>>
>>>> The set_fmt callback chooses among these 3 configurations the one that
>>>> matches the requested format.
>>>>
>>>> Adding support to VIDIOC_SUBDEV_G_SELECTION and
>>>> VIDIOC_SUBDEV_S_SELECTION involved a complete rewrite of set_fmt,
>>>> which now chooses the most appropriate binning based on the ratio
>>>> between the previously-set cropping size and the format size being
>>>> requested.
>>>>
>>>> Note that the names in enum imx274_mode change from being
>>>> resolution-based to being binning-based. Without cropping, the two
>>>> naming are equivalent. With cropping, the resolution could be
>>>> different, e.g. using 2:1 binning mode to crop 1200x960 and output a
>>>> 600x480 format. Using binning in the names avoids anny
>>>> misunderstanding.
>>>>
>>>> Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
>>>>
>>>> ---
>>>> Changed v1 -> v2:
>>>>  - add "media: " prefix to commit message
>>>> ---
>>>>  drivers/media/i2c/imx274.c | 266 
>>>> -
>>>>  1 file changed, 192 insertions(+), 74 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
>>>> index b6c54712f2aa..ceb5b3e498c6 100644
>>>> --- a/drivers/media/i2c/imx274.c
>>>> +++ b/drivers/media/i2c/imx274.c
>>>> @@ -19,6 +19,7 @@
>>>>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>>   */
>>>>  
>>>> +#include 
>>>>  #include 
>>>>  #include 
>>>>  #include 
>>>> @@ -74,7 +75,7 @@
>>>>   */
>>>>  #define IMX274_MIN_EXPOSURE_TIME  (4 * 260 / 72)
>>>>  
>>>> -#define IMX274_DEFAULT_MODE   IMX274_MODE_3840X2160
>>>> +#define IMX274_DEFAULT_MODE   IMX274_MODE_BINNING_OFF
>>>>  #define IMX274_MAX_WIDTH  (3840)
>>>>  #define IMX274_MAX_HEIGHT (2160)
>>>>  #define IMX274_MAX_FRAME_RATE (120)
>>>> @@ -111,6 +112,20 @@
>>>>  #define IMX274_SHR_REG_LSB0x300C /* SHR */
>>>>  #define IMX274_SVR_REG_MSB0x300F /* SVR */
>>>>  #define IMX274_SVR_REG_LSB0x300E /* SVR */
>>>> +#define IMX274_HTRIM_EN_REG   0x3037
>>>> +#define IMX274_HTRIM_START_REG_LSB0x3038
>>>> +#define IMX274_HTRIM_START_REG_MSB0x3039
>>>> +#define IMX274_HTRIM_END_REG_LSB  0x303A
>>>> +#define IMX274_HTRIM_END_REG_MSB  0x303B
>>>> +#define IMX274_VWIDCUTEN_REG  0x30DD
>>>> +#define IMX274_VWIDCUT_REG_LSB0x30DE
>>>> +#define IMX274_VWIDCUT_REG_MSB0x30DF
>>>> +#define IMX274_VWINPOS_REG_LSB0x30E0
>>>> +#define IMX274_VWINPOS_REG_MSB0x30E1
>>>> +#define IMX274_WRITE_VSIZE_REG_LSB0x3130
>>>> +#define IMX274_WRITE_VSIZE_REG_MSB0x3131
&g

Re: [PATCH v2 13/13] media: imx274: add SELECTION support for cropping

[Luca Ceresoli]

Hi Sakari,

On 24/04/2018 11:59, Sakari Ailus wrote:
> Hi Luca,
> 
> Thank you for the patchset.
> 
> Some comments below... what I propose is that I apply the rest of the
> patches and then the comments to this one could be addressed separately.
> Would that work for you?

Yes, but I suggest you only apply patches 1-6. I noticed a warning in
patch 9, and patches 7-8 are not very useful without it. Will fix it in v3.

I'll wait for the outcome of the discussion below before sending v3.
Tell me if you prefer me to do it earlier.

> On Tue, Apr 24, 2018 at 10:24:18AM +0200, Luca Ceresoli wrote:
>> Currently this driver does not support cropping. The supported modes
>> are the following, all capturing the entire area:
>>
>>  - 3840x2160, 1:1 binning (native sensor resolution)
>>  - 1920x1080, 2:1 binning
>>  - 1280x720,  3:1 binning
>>
>> The set_fmt callback chooses among these 3 configurations the one that
>> matches the requested format.
>>
>> Adding support to VIDIOC_SUBDEV_G_SELECTION and
>> VIDIOC_SUBDEV_S_SELECTION involved a complete rewrite of set_fmt,
>> which now chooses the most appropriate binning based on the ratio
>> between the previously-set cropping size and the format size being
>> requested.
>>
>> Note that the names in enum imx274_mode change from being
>> resolution-based to being binning-based. Without cropping, the two
>> naming are equivalent. With cropping, the resolution could be
>> different, e.g. using 2:1 binning mode to crop 1200x960 and output a
>> 600x480 format. Using binning in the names avoids anny
>> misunderstanding.
>>
>> Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
>>
>> ---
>> Changed v1 -> v2:
>>  - add "media: " prefix to commit message
>> ---
>>  drivers/media/i2c/imx274.c | 266 
>> -
>>  1 file changed, 192 insertions(+), 74 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
>> index b6c54712f2aa..ceb5b3e498c6 100644
>> --- a/drivers/media/i2c/imx274.c
>> +++ b/drivers/media/i2c/imx274.c
>> @@ -19,6 +19,7 @@
>>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>   */
>>  
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -74,7 +75,7 @@
>>   */
>>  #define IMX274_MIN_EXPOSURE_TIME(4 * 260 / 72)
>>  
>> -#define IMX274_DEFAULT_MODE IMX274_MODE_3840X2160
>> +#define IMX274_DEFAULT_MODE IMX274_MODE_BINNING_OFF
>>  #define IMX274_MAX_WIDTH(3840)
>>  #define IMX274_MAX_HEIGHT   (2160)
>>  #define IMX274_MAX_FRAME_RATE   (120)
>> @@ -111,6 +112,20 @@
>>  #define IMX274_SHR_REG_LSB  0x300C /* SHR */
>>  #define IMX274_SVR_REG_MSB  0x300F /* SVR */
>>  #define IMX274_SVR_REG_LSB  0x300E /* SVR */
>> +#define IMX274_HTRIM_EN_REG 0x3037
>> +#define IMX274_HTRIM_START_REG_LSB  0x3038
>> +#define IMX274_HTRIM_START_REG_MSB  0x3039
>> +#define IMX274_HTRIM_END_REG_LSB0x303A
>> +#define IMX274_HTRIM_END_REG_MSB0x303B
>> +#define IMX274_VWIDCUTEN_REG0x30DD
>> +#define IMX274_VWIDCUT_REG_LSB  0x30DE
>> +#define IMX274_VWIDCUT_REG_MSB  0x30DF
>> +#define IMX274_VWINPOS_REG_LSB  0x30E0
>> +#define IMX274_VWINPOS_REG_MSB  0x30E1
>> +#define IMX274_WRITE_VSIZE_REG_LSB  0x3130
>> +#define IMX274_WRITE_VSIZE_REG_MSB  0x3131
>> +#define IMX274_Y_OUT_SIZE_REG_LSB   0x3132
>> +#define IMX274_Y_OUT_SIZE_REG_MSB   0x3133
>>  #define IMX274_VMAX_REG_1   0x30FA /* VMAX, MSB */
>>  #define IMX274_VMAX_REG_2   0x30F9 /* VMAX */
>>  #define IMX274_VMAX_REG_3   0x30F8 /* VMAX, LSB */
>> @@ -141,9 +156,9 @@ static const struct regmap_config imx274_regmap_config = 
>> {
>>  };
>>  
>>  enum imx274_mode {
>> -IMX274_MODE_3840X2160,
>> -IMX274_MODE_1920X1080,
>> -IMX274_MODE_1280X720,
>> +IMX274_MODE_BINNING_OFF,
>> +IMX274_MODE_BINNING_2_1,
>> +IMX274_MODE_BINNING_3_1,
>>  };
>>  
>>  /*
>> @@ -215,31 +230,14 @@ static const struct reg_8 
>> imx274_mode1_3840x2160_raw10[] = {
>>  {0x3004, 0x01},
>>  {0x3005, 0x01},
>>  {0x3006, 0x00},
>> -{0x3007,

[PATCH v2 05/13] media: imx274: rename and reorder register address definitions

[Luca Ceresoli]

Most registers are defined using the name used in the datasheet.
E.g. the defines for the HMAX register are IMX274_HMAX_REG_*.

Rename the SHR and VMAX register accordingly. Also move them close to
related registers: SHR close to SVR, VMAX close to HMAX.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>

---
Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 26 +-
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 9203d377cfe2..62a0d7af0e51 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -107,15 +107,15 @@
 /*
  * IMX274 register definitions
  */
-#define IMX274_FRAME_LENGTH_ADDR_1 0x30FA /* VMAX, MSB */
-#define IMX274_FRAME_LENGTH_ADDR_2 0x30F9 /* VMAX */
-#define IMX274_FRAME_LENGTH_ADDR_3 0x30F8 /* VMAX, LSB */
+#define IMX274_SHR_REG_MSB 0x300D /* SHR */
+#define IMX274_SHR_REG_LSB 0x300C /* SHR */
 #define IMX274_SVR_REG_MSB 0x300F /* SVR */
 #define IMX274_SVR_REG_LSB 0x300E /* SVR */
+#define IMX274_VMAX_REG_1  0x30FA /* VMAX, MSB */
+#define IMX274_VMAX_REG_2  0x30F9 /* VMAX */
+#define IMX274_VMAX_REG_3  0x30F8 /* VMAX, LSB */
 #define IMX274_HMAX_REG_MSB0x30F7 /* HMAX */
 #define IMX274_HMAX_REG_LSB0x30F6 /* HMAX */
-#define IMX274_COARSE_TIME_ADDR_MSB0x300D /* SHR */
-#define IMX274_COARSE_TIME_ADDR_LSB0x300C /* SHR */
 #define IMX274_ANALOG_GAIN_ADDR_LSB0x300A /* ANALOG GAIN LSB */
 #define IMX274_ANALOG_GAIN_ADDR_MSB0x300B /* ANALOG GAIN MSB */
 #define IMX274_DIGITAL_GAIN_REG0x3012 /* Digital Gain 
*/
@@ -1126,15 +1126,15 @@ static int imx274_get_frame_length(struct stimx274 
*priv, u32 *val)
svr = (reg_val[1] << IMX274_SHIFT_8_BITS) + reg_val[0];
 
/* vmax */
-   err = imx274_read_reg(priv, IMX274_FRAME_LENGTH_ADDR_3, _val[0]);
+   err = imx274_read_reg(priv, IMX274_VMAX_REG_3, _val[0]);
if (err)
goto fail;
 
-   err = imx274_read_reg(priv, IMX274_FRAME_LENGTH_ADDR_2, _val[1]);
+   err = imx274_read_reg(priv, IMX274_VMAX_REG_2, _val[1]);
if (err)
goto fail;
 
-   err = imx274_read_reg(priv, IMX274_FRAME_LENGTH_ADDR_1, _val[2]);
+   err = imx274_read_reg(priv, IMX274_VMAX_REG_1, _val[2]);
if (err)
goto fail;
 
@@ -1293,10 +1293,10 @@ static int imx274_set_gain(struct stimx274 *priv, 
struct v4l2_ctrl *ctrl)
 static inline void imx274_calculate_coarse_time_regs(struct reg_8 regs[2],
 u32 coarse_time)
 {
-   regs->addr = IMX274_COARSE_TIME_ADDR_MSB;
+   regs->addr = IMX274_SHR_REG_MSB;
regs->val = (coarse_time >> IMX274_SHIFT_8_BITS)
& IMX274_MASK_LSB_8_BITS;
-   (regs + 1)->addr = IMX274_COARSE_TIME_ADDR_LSB;
+   (regs + 1)->addr = IMX274_SHR_REG_LSB;
(regs + 1)->val = (coarse_time) & IMX274_MASK_LSB_8_BITS;
 }
 
@@ -1464,13 +1464,13 @@ static int imx274_set_test_pattern(struct stimx274 
*priv, int val)
 static inline void imx274_calculate_frame_length_regs(struct reg_8 regs[3],
  u32 frame_length)
 {
-   regs->addr = IMX274_FRAME_LENGTH_ADDR_1;
+   regs->addr = IMX274_VMAX_REG_1;
regs->val = (frame_length >> IMX274_SHIFT_16_BITS)
& IMX274_MASK_LSB_4_BITS;
-   (regs + 1)->addr = IMX274_FRAME_LENGTH_ADDR_2;
+   (regs + 1)->addr = IMX274_VMAX_REG_2;
(regs + 1)->val = (frame_length >> IMX274_SHIFT_8_BITS)
& IMX274_MASK_LSB_8_BITS;
-   (regs + 2)->addr = IMX274_FRAME_LENGTH_ADDR_3;
+   (regs + 2)->addr = IMX274_VMAX_REG_3;
(regs + 2)->val = (frame_length) & IMX274_MASK_LSB_8_BITS;
 }
 
-- 
2.7.4

[PATCH v2 06/13] media: imx274: remove non-indexed pointers from mode_table

[Luca Ceresoli]

mode_table[] has 3 members that are accessed based on their index, which
makes worth using an array.

The other members are always accessed with a constant index. This added
indirection gives no improvement and only makes code more verbose.

Remove these pointers from the array and access them directly.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>

---
Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 25 ++---
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 62a0d7af0e51..63fb94e7da37 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -144,12 +144,6 @@ enum imx274_mode {
IMX274_MODE_3840X2160,
IMX274_MODE_1920X1080,
IMX274_MODE_1280X720,
-
-   IMX274_MODE_START_STREAM_1,
-   IMX274_MODE_START_STREAM_2,
-   IMX274_MODE_START_STREAM_3,
-   IMX274_MODE_START_STREAM_4,
-   IMX274_MODE_STOP_STREAM
 };
 
 /*
@@ -486,12 +480,6 @@ static const struct reg_8 *mode_table[] = {
[IMX274_MODE_3840X2160] = imx274_mode1_3840x2160_raw10,
[IMX274_MODE_1920X1080] = imx274_mode3_1920x1080_raw10,
[IMX274_MODE_1280X720]  = imx274_mode5_1280x720_raw10,
-
-   [IMX274_MODE_START_STREAM_1]= imx274_start_1,
-   [IMX274_MODE_START_STREAM_2]= imx274_start_2,
-   [IMX274_MODE_START_STREAM_3]= imx274_start_3,
-   [IMX274_MODE_START_STREAM_4]= imx274_start_4,
-   [IMX274_MODE_STOP_STREAM]   = imx274_stop,
 };
 
 /*
@@ -731,11 +719,11 @@ static int imx274_mode_regs(struct stimx274 *priv, int 
mode)
 {
int err = 0;
 
-   err = imx274_write_table(priv, mode_table[IMX274_MODE_START_STREAM_1]);
+   err = imx274_write_table(priv, imx274_start_1);
if (err)
return err;
 
-   err = imx274_write_table(priv, mode_table[IMX274_MODE_START_STREAM_2]);
+   err = imx274_write_table(priv, imx274_start_2);
if (err)
return err;
 
@@ -760,7 +748,7 @@ static int imx274_start_stream(struct stimx274 *priv)
 * give it 1 extra ms for margin
 */
msleep_range(11);
-   err = imx274_write_table(priv, mode_table[IMX274_MODE_START_STREAM_3]);
+   err = imx274_write_table(priv, imx274_start_3);
if (err)
return err;
 
@@ -770,7 +758,7 @@ static int imx274_start_stream(struct stimx274 *priv)
 * give it 1 extra ms for margin
 */
msleep_range(8);
-   err = imx274_write_table(priv, mode_table[IMX274_MODE_START_STREAM_4]);
+   err = imx274_write_table(priv, imx274_start_4);
if (err)
return err;
 
@@ -1081,8 +1069,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
goto fail;
} else {
/* stop stream */
-   ret = imx274_write_table(imx274,
-mode_table[IMX274_MODE_STOP_STREAM]);
+   ret = imx274_write_table(imx274, imx274_stop);
if (ret)
goto fail;
}
@@ -1779,7 +1766,7 @@ static int imx274_remove(struct i2c_client *client)
struct stimx274 *imx274 = to_imx274(sd);
 
/* stop stream */
-   imx274_write_table(imx274, mode_table[IMX274_MODE_STOP_STREAM]);
+   imx274_write_table(imx274, imx274_stop);
 
v4l2_async_unregister_subdev(sd);
v4l2_ctrl_handler_free(>ctrls.handler);
-- 
2.7.4

[PATCH v2 00/13] media: imx274: cleanups, improvements and SELECTION API support

[Luca Ceresoli]

Hi,

this patchset introduces cropping support for the Sony IMX274 sensor
using the SELECTION API.

With respect to v1 this patchset adds the "media: " prefix to commits
and fixes a warning in patch 9.

Patches 1-6 clean up and restructure code in various places and are
pretty much independent from the cropping feature.

Patches 7-11 are further restructuring which are mostly useful to
implement cropping API in a cleaner way.

Patch 12 introduces a helper to allow setting many registers computed
at runtime in a straightforward way. This would not have been very
useful before because all long register write sequences came from
const tables, but it's definitely a must for the cropping code.

Patch 13 implements cropping in the set_selection pad operation. On
the v4l2 side there is nothing special. The most tricky part was
respecting all the device constraints on the horizontal crop.

Regards,
Luca

Luca Ceresoli (13):
  media: imx274: document reset delays more clearly
  media: imx274: fix typo in comment
  media: imx274: slightly simplify code
  media: imx274: remove unused data from struct imx274_frmfmt
  media: imx274: rename and reorder register address definitions
  media: imx274: remove non-indexed pointers from mode_table
  media: imx274: initialize format before v4l2 controls
  media: imx274: consolidate per-mode data in imx274_frmfmt
  media: imx274: get rid of mode_index
  media: imx274: actually use IMX274_DEFAULT_MODE
  media: imx274: simplify imx274_write_table()
  media: imx274: add helper function to fill a reg_8 table chunk
  media: imx274: add SELECTION support for cropping

 drivers/media/i2c/imx274.c | 598 ++---
 1 file changed, 348 insertions(+), 250 deletions(-)

-- 
2.7.4

[PATCH v2 04/13] media: imx274: remove unused data from struct imx274_frmfmt

[Luca Ceresoli]

struct imx274_frmfmt is instantiated only in the imx274_formats[]
array, where imx274_formats[N].mode always equals N (via enum
imx274_mode).  So .mode carries no information, and unsurprisingly it
is never used.

mbus_code is never used because the 12 bit modes are not implemented.

The colorspace member is also never used, which is normal since the
imx274 sensor can output only one colorspace.

Let's get rid of all of them.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>

---
Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 17 +
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index c5d00ade4d64..9203d377cfe2 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -156,10 +156,7 @@ enum imx274_mode {
  * imx274 format related structure
  */
 struct imx274_frmfmt {
-   u32 mbus_code;
-   enum v4l2_colorspace colorspace;
struct v4l2_frmsize_discrete size;
-   enum imx274_mode mode;
 };
 
 /*
@@ -501,12 +498,9 @@ static const struct reg_8 *mode_table[] = {
  * imx274 format related structure
  */
 static const struct imx274_frmfmt imx274_formats[] = {
-   {MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB, {3840, 2160},
-   IMX274_MODE_3840X2160},
-   {MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB, {1920, 1080},
-   IMX274_MODE_1920X1080},
-   {MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB, {1280, 720},
-   IMX274_MODE_1280X720},
+   { {3840, 2160} },
+   { {1920, 1080} },
+   { {1280,  720} },
 };
 
 /*
@@ -890,9 +884,8 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
int index;
 
dev_dbg(>dev,
-   "%s: width = %d height = %d code = %d mbus_code = %d\n",
-   __func__, fmt->width, fmt->height, fmt->code,
-   imx274_formats[imx274->mode_index].mbus_code);
+   "%s: width = %d height = %d code = %d\n",
+   __func__, fmt->width, fmt->height, fmt->code);
 
mutex_lock(>lock);
 
-- 
2.7.4

[PATCH v2 08/13] media: imx274: consolidate per-mode data in imx274_frmfmt

[Luca Ceresoli]

Data about the implemented readout modes is partially stored in
imx274_formats[], the rest is scattered in several arrays. The latter
are then accessed using the mode index, e.g.:

  min_frame_len[priv->mode_index]

Consolidate all these data in imx274_formats[], and store a pointer to
the selected mode (i.e. imx274_formats[priv->mode_index]) in the main
device struct. This way code to use these data becomes more readable,
e.g.:

  priv->mode->min_frame_len

This removes lots of scaffolding code and keeps data about each mode
in a unique place.

Also remove a parameter to imx274_mode_regs() that is now unused.

While this adds the mode pointer to the device struct, it does not
remove the mode_index from it because mode_index is still used in two
dev_dbg() calls.  This will be handled in a follow-up commit.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>

---
Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 139 +
 1 file changed, 66 insertions(+), 73 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 8a8a11b8d75d..2ec31ae4e60d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -147,10 +147,28 @@ enum imx274_mode {
 };
 
 /*
- * imx274 format related structure
+ * Parameters for each imx274 readout mode.
+ *
+ * These are the values to configure the sensor in one of the
+ * implemented modes.
+ *
+ * @size: recommended recording pixels
+ * @init_regs: registers to initialize the mode
+ * @min_frame_len: Minimum frame length for each mode (see "Frame Rate
+ * Adjustment (CSI-2)" in the datasheet)
+ * @min_SHR: Minimum SHR register value (see "Shutter Setting (CSI-2)" in the
+ *   datasheet)
+ * @max_fps: Maximum frames per second
+ * @nocpiop: Number of clocks per internal offset period (see "Integration Time
+ *   in Each Readout Drive Mode (CSI-2)" in the datasheet)
  */
 struct imx274_frmfmt {
struct v4l2_frmsize_discrete size;
+   const struct reg_8 *init_regs;
+   int min_frame_len;
+   int min_SHR;
+   int max_fps;
+   int nocpiop;
 };
 
 /*
@@ -476,58 +494,35 @@ static const struct reg_8 imx274_tp_regs[] = {
{IMX274_TABLE_END, 0x00}
 };
 
-static const struct reg_8 *mode_table[] = {
-   [IMX274_MODE_3840X2160] = imx274_mode1_3840x2160_raw10,
-   [IMX274_MODE_1920X1080] = imx274_mode3_1920x1080_raw10,
-   [IMX274_MODE_1280X720]  = imx274_mode5_1280x720_raw10,
-};
-
-/*
- * imx274 format related structure
- */
+/* nocpiop happens to be the same number for the implemented modes */
 static const struct imx274_frmfmt imx274_formats[] = {
-   { {3840, 2160} },
-   { {1920, 1080} },
-   { {1280,  720} },
-};
-
-/*
- * minimal frame length for each mode
- * refer to datasheet section "Frame Rate Adjustment (CSI-2)"
- */
-static const int min_frame_len[] = {
-   4550, /* mode 1, 4K */
-   2310, /* mode 3, 1080p */
-   2310 /* mode 5, 720p */
-};
-
-/*
- * minimal numbers of SHR register
- * refer to datasheet table "Shutter Setting (CSI-2)"
- */
-static const int min_SHR[] = {
-   12, /* mode 1, 4K */
-   8, /* mode 3, 1080p */
-   8 /* mode 5, 720p */
-};
-
-static const int max_frame_rate[] = {
-   60, /* mode 1 , 4K */
-   120, /* mode 3, 1080p */
-   120 /* mode 5, 720p */
-};
-
-/*
- * Number of clocks per internal offset period
- * a constant based on mode
- * refer to section "Integration Time in Each Readout Drive Mode (CSI-2)"
- * in the datasheet
- * for the implemented 3 modes, it happens to be the same number
- */
-static const int nocpiop[] = {
-   112, /* mode 1 , 4K */
-   112, /* mode 3, 1080p */
-   112 /* mode 5, 720p */
+   {
+   /* mode 1, 4K */
+   .size = {3840, 2160},
+   .init_regs = imx274_mode1_3840x2160_raw10,
+   .min_frame_len = 4550,
+   .min_SHR = 12,
+   .max_fps = 60,
+   .nocpiop = 112,
+   },
+   {
+   /* mode 3, 1080p */
+   .size = {1920, 1080},
+   .init_regs = imx274_mode3_1920x1080_raw10,
+   .min_frame_len = 2310,
+   .min_SHR = 8,
+   .max_fps = 120,
+   .nocpiop = 112,
+   },
+   {
+   /* mode 5, 720p */
+   .size = {1280, 720},
+   .init_regs = imx274_mode5_1280x720_raw10,
+   .min_frame_len = 2310,
+   .min_SHR = 8,
+   .max_fps = 120,
+   .nocpiop = 112,
+   },
 };
 
 /*
@@ -557,6 +552,8 @@ struct imx274_ctrls {
  * @regmap: Pointer to regmap structure
  * @reset_gpio: Pointer to reset gpio
  * @lock: Mutex structure
+ * @mode: Parameters for the selected readout mode
+ *(points 

[PATCH v2 11/13] media: imx274: simplify imx274_write_table()

[Luca Ceresoli]

imx274_write_table() is a mere wrapper (and the only user) to
imx274_regmap_util_write_table_8(). Remove this useless indirection by
merging the two functions into one.

Also get rid of the wait_ms_addr and end_addr parameters since it does
not make any sense to give them any values other than
IMX274_TABLE_WAIT_MS and IMX274_TABLE_END.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>

---
Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 28 ++--
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index ae90fb31416c..bc307eb65a10 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -597,20 +597,18 @@ static inline struct stimx274 *to_imx274(struct 
v4l2_subdev *sd)
 }
 
 /*
- * imx274_regmap_util_write_table_8 - Function for writing register table
- * @regmap: Pointer to device reg map structure
- * @table: Table containing register values
- * @wait_ms_addr: Flag for performing delay
- * @end_addr: Flag for incating end of table
+ * Writing a register table
+ *
+ * @priv: Pointer to device
+ * @table: Table containing register values (with optional delays)
  *
  * This is used to write register table into sensor's reg map.
  *
  * Return: 0 on success, errors otherwise
  */
-static int imx274_regmap_util_write_table_8(struct regmap *regmap,
-   const struct reg_8 table[],
-   u16 wait_ms_addr, u16 end_addr)
+static int imx274_write_table(struct stimx274 *priv, const struct reg_8 
table[])
 {
+   struct regmap *regmap = priv->regmap;
int err = 0;
const struct reg_8 *next;
u8 val;
@@ -622,8 +620,8 @@ static int imx274_regmap_util_write_table_8(struct regmap 
*regmap,
 
for (next = table;; next++) {
if ((next->addr != range_start + range_count) ||
-   (next->addr == end_addr) ||
-   (next->addr == wait_ms_addr) ||
+   (next->addr == IMX274_TABLE_END) ||
+   (next->addr == IMX274_TABLE_WAIT_MS) ||
(range_count == max_range_vals)) {
if (range_count == 1)
err = regmap_write(regmap,
@@ -642,10 +640,10 @@ static int imx274_regmap_util_write_table_8(struct regmap 
*regmap,
range_count = 0;
 
/* Handle special address values */
-   if (next->addr == end_addr)
+   if (next->addr == IMX274_TABLE_END)
break;
 
-   if (next->addr == wait_ms_addr) {
+   if (next->addr == IMX274_TABLE_WAIT_MS) {
msleep_range(next->val);
continue;
}
@@ -692,12 +690,6 @@ static inline int imx274_write_reg(struct stimx274 *priv, 
u16 addr, u8 val)
return err;
 }
 
-static int imx274_write_table(struct stimx274 *priv, const struct reg_8 
table[])
-{
-   return imx274_regmap_util_write_table_8(priv->regmap,
-   table, IMX274_TABLE_WAIT_MS, IMX274_TABLE_END);
-}
-
 /*
  * Set mode registers to start stream.
  * @priv: Pointer to device structure
-- 
2.7.4

[PATCH v2 13/13] media: imx274: add SELECTION support for cropping

[Luca Ceresoli]

Currently this driver does not support cropping. The supported modes
are the following, all capturing the entire area:

 - 3840x2160, 1:1 binning (native sensor resolution)
 - 1920x1080, 2:1 binning
 - 1280x720,  3:1 binning

The set_fmt callback chooses among these 3 configurations the one that
matches the requested format.

Adding support to VIDIOC_SUBDEV_G_SELECTION and
VIDIOC_SUBDEV_S_SELECTION involved a complete rewrite of set_fmt,
which now chooses the most appropriate binning based on the ratio
between the previously-set cropping size and the format size being
requested.

Note that the names in enum imx274_mode change from being
resolution-based to being binning-based. Without cropping, the two
naming are equivalent. With cropping, the resolution could be
different, e.g. using 2:1 binning mode to crop 1200x960 and output a
600x480 format. Using binning in the names avoids anny
misunderstanding.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>

---
Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 266 -
 1 file changed, 192 insertions(+), 74 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index b6c54712f2aa..ceb5b3e498c6 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -19,6 +19,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -74,7 +75,7 @@
  */
 #define IMX274_MIN_EXPOSURE_TIME   (4 * 260 / 72)
 
-#define IMX274_DEFAULT_MODEIMX274_MODE_3840X2160
+#define IMX274_DEFAULT_MODEIMX274_MODE_BINNING_OFF
 #define IMX274_MAX_WIDTH   (3840)
 #define IMX274_MAX_HEIGHT  (2160)
 #define IMX274_MAX_FRAME_RATE  (120)
@@ -111,6 +112,20 @@
 #define IMX274_SHR_REG_LSB 0x300C /* SHR */
 #define IMX274_SVR_REG_MSB 0x300F /* SVR */
 #define IMX274_SVR_REG_LSB 0x300E /* SVR */
+#define IMX274_HTRIM_EN_REG0x3037
+#define IMX274_HTRIM_START_REG_LSB 0x3038
+#define IMX274_HTRIM_START_REG_MSB 0x3039
+#define IMX274_HTRIM_END_REG_LSB   0x303A
+#define IMX274_HTRIM_END_REG_MSB   0x303B
+#define IMX274_VWIDCUTEN_REG   0x30DD
+#define IMX274_VWIDCUT_REG_LSB 0x30DE
+#define IMX274_VWIDCUT_REG_MSB 0x30DF
+#define IMX274_VWINPOS_REG_LSB 0x30E0
+#define IMX274_VWINPOS_REG_MSB 0x30E1
+#define IMX274_WRITE_VSIZE_REG_LSB 0x3130
+#define IMX274_WRITE_VSIZE_REG_MSB 0x3131
+#define IMX274_Y_OUT_SIZE_REG_LSB  0x3132
+#define IMX274_Y_OUT_SIZE_REG_MSB  0x3133
 #define IMX274_VMAX_REG_1  0x30FA /* VMAX, MSB */
 #define IMX274_VMAX_REG_2  0x30F9 /* VMAX */
 #define IMX274_VMAX_REG_3  0x30F8 /* VMAX, LSB */
@@ -141,9 +156,9 @@ static const struct regmap_config imx274_regmap_config = {
 };
 
 enum imx274_mode {
-   IMX274_MODE_3840X2160,
-   IMX274_MODE_1920X1080,
-   IMX274_MODE_1280X720,
+   IMX274_MODE_BINNING_OFF,
+   IMX274_MODE_BINNING_2_1,
+   IMX274_MODE_BINNING_3_1,
 };
 
 /*
@@ -215,31 +230,14 @@ static const struct reg_8 imx274_mode1_3840x2160_raw10[] 
= {
{0x3004, 0x01},
{0x3005, 0x01},
{0x3006, 0x00},
-   {0x3007, 0x02},
+   {0x3007, 0xa2},
 
{0x3018, 0xA2}, /* output XVS, HVS */
 
{0x306B, 0x05},
{0x30E2, 0x01},
-   {0x30F6, 0x07}, /* HMAX, 263 */
-   {0x30F7, 0x01}, /* HMAX */
-
-   {0x30dd, 0x01}, /* crop to 2160 */
-   {0x30de, 0x06},
-   {0x30df, 0x00},
-   {0x30e0, 0x12},
-   {0x30e1, 0x00},
-   {0x3037, 0x01}, /* to crop to 3840 */
-   {0x3038, 0x0c},
-   {0x3039, 0x00},
-   {0x303a, 0x0c},
-   {0x303b, 0x0f},
 
{0x30EE, 0x01},
-   {0x3130, 0x86},
-   {0x3131, 0x08},
-   {0x3132, 0x7E},
-   {0x3133, 0x08},
{0x3342, 0x0A},
{0x3343, 0x00},
{0x3344, 0x16},
@@ -273,7 +271,7 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] = {
{0x3004, 0x02},
{0x3005, 0x21},
{0x3006, 0x00},
-   {0x3007, 0x11},
+   {0x3007, 0xb1},
 
{0x3018, 0xA2}, /* output XVS, HVS */
 
@@ -283,22 +281,7 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] = 
{
{0x30F6, 0x04}, /* HMAX, 260 */
{0x30F7, 0x01}, /* HMAX */
 
-   {0x30dd, 0x01}, /* to crop to 1920x1080 */
-   {0x30de, 0x05},
-   {0x30df, 0x00},
-   {0x30e0, 0x04},
-   {0x30e1, 0x00},
-   {0x3037, 0x01},
-   {0x3038, 0x0c},
-   {0x3039, 0x00},
-   {0x303a, 0x0c},
-   {0x303b, 0x0f},
-
{0x30EE, 0x01},
-   {0x3130, 0x4

[PATCH v2 12/13] media: imx274: add helper function to fill a reg_8 table chunk

[Luca Ceresoli]

Tables of struct reg_8 are used to simplify multi-byte register
assignment. However filling these snippets with values computed at
runtime is currently implemented by very similar functions doing the
needed shift & mask manipulation.

Replace all those functions with a unique helper function to fill
reg_8 tables in a simple and clean way.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>

---
Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 90 --
 1 file changed, 55 insertions(+), 35 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index bc307eb65a10..b6c54712f2aa 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -597,6 +597,58 @@ static inline struct stimx274 *to_imx274(struct 
v4l2_subdev *sd)
 }
 
 /*
+ * Fill consecutive reg_8 items in order to set a multibyte register.
+ *
+ * The sensor has many 2-bytes registers and a 3-byte register. This
+ * simplifies code to set them by filling consecutive entries of a
+ * struct reg_8 table with the data to set a register.
+ *
+ * The number of table entries that is filled is the minimum needed
+ * for the given number of bits (i.e. nbits / 8, rounded up). If nbits
+ * is not a multiple of 8, extra bits in the most significant byte are
+ * zeroed.
+ *
+ * Example:
+ *   Calling prepare_reg([10], 0x3000, 0xcafe, 12) will set:
+ *   regs[10] = { .addr = 0x3000, .val = 0xfe }
+ *   regs[11] = { .addr = 0x3001, .val = 0x0a }
+ *
+ * @table_base: Pointer to the first reg_8 struct to be filled.  The
+ *  following entries will also be set, make sure they are
+ *  properly allocated!
+ * @addr_lsb: Address of the LSB register.  Other registers must be
+ * consecutive, least-to-most significant.
+ * @value: Value to be written to the register.
+ * @nbits: Number of bits to write (range: [1..24])
+ */
+static void prepare_reg(struct reg_8 *table_base,
+   u16 addr_lsb,
+   u32 value,
+   size_t nbits)
+{
+   struct reg_8 *cmd = table_base;
+   u16 addr = addr_lsb;
+   size_t bits; /* how many bits at this round */
+
+   WARN_ON(nbits > 24);
+
+   if (nbits > 24)
+   nbits = 24;
+
+   while (nbits > 0) {
+   bits = min_t(size_t, 8, nbits);
+
+   cmd->addr = addr;
+   cmd->val = value & ((1 << bits) - 1);
+
+   nbits -= bits;
+   value >>= 8;
+   addr++;
+   cmd++;
+   }
+}
+
+/*
  * Writing a register table
  *
  * @priv: Pointer to device
@@ -1163,15 +1215,6 @@ static int imx274_set_digital_gain(struct stimx274 
*priv, u32 dgain)
reg_val & IMX274_MASK_LSB_4_BITS);
 }
 
-static inline void imx274_calculate_gain_regs(struct reg_8 regs[2], u16 gain)
-{
-   regs->addr = IMX274_ANALOG_GAIN_ADDR_MSB;
-   regs->val = (gain >> IMX274_SHIFT_8_BITS) & IMX274_MASK_LSB_3_BITS;
-
-   (regs + 1)->addr = IMX274_ANALOG_GAIN_ADDR_LSB;
-   (regs + 1)->val = (gain) & IMX274_MASK_LSB_8_BITS;
-}
-
 /*
  * imx274_set_gain - Function called when setting gain
  * @priv: Pointer to device structure
@@ -1229,7 +1272,7 @@ static int imx274_set_gain(struct stimx274 *priv, struct 
v4l2_ctrl *ctrl)
if (gain_reg > IMX274_GAIN_REG_MAX)
gain_reg = IMX274_GAIN_REG_MAX;
 
-   imx274_calculate_gain_regs(reg_list, (u16)gain_reg);
+   prepare_reg(reg_list, IMX274_ANALOG_GAIN_ADDR_LSB, gain_reg, 11);
 
for (i = 0; i < ARRAY_SIZE(reg_list); i++) {
err = imx274_write_reg(priv, reg_list[i].addr,
@@ -1258,16 +1301,6 @@ static int imx274_set_gain(struct stimx274 *priv, struct 
v4l2_ctrl *ctrl)
return err;
 }
 
-static inline void imx274_calculate_coarse_time_regs(struct reg_8 regs[2],
-u32 coarse_time)
-{
-   regs->addr = IMX274_SHR_REG_MSB;
-   regs->val = (coarse_time >> IMX274_SHIFT_8_BITS)
-   & IMX274_MASK_LSB_8_BITS;
-   (regs + 1)->addr = IMX274_SHR_REG_LSB;
-   (regs + 1)->val = (coarse_time) & IMX274_MASK_LSB_8_BITS;
-}
-
 /*
  * imx274_set_coarse_time - Function called when setting SHR value
  * @priv: Pointer to device structure
@@ -1292,7 +1325,7 @@ static int imx274_set_coarse_time(struct stimx274 *priv, 
u32 *val)
goto fail;
 
/* prepare SHR registers */
-   imx274_calculate_coarse_time_regs(reg_list, coarse_time);
+   prepare_reg(reg_list, IMX274_SHR_REG_LSB, coarse_time, 16);
 
/* write to SHR registers */
for (i = 0; i < ARRAY_SIZE(reg_list); i++) {
@@ -1429,19 +1462,6 @@ static int imx274_set_test_pattern(struct stimx2

[PATCH v2 09/13] media: imx274: get rid of mode_index

[Luca Ceresoli]

After restructuring struct imx274_frmfmt, the mode_index field is
still in use only for two dev_dbg() calls in imx274_s_stream(). Let's
remove it and avoid duplicated information.

Replacing the first usage requires a some rather annoying but trivial
pointer math. The other one can be removed entirely since it would
print the same value anyway.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>

---
Changed v1 -> v2:
 - add "media: " prefix to commit message
 - fix dev_dbg() format mismatch warning
   ("warning: format ‘%ld’ expects argument of type ‘long int’, but argument 6 
has type ‘int’")
 - slightly improve commit message
---
 drivers/media/i2c/imx274.c | 13 -
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 2ec31ae4e60d..69fdc82d4214 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -553,8 +553,6 @@ struct imx274_ctrls {
  * @reset_gpio: Pointer to reset gpio
  * @lock: Mutex structure
  * @mode: Parameters for the selected readout mode
- *(points to imx274_formats[mode_index])
- * @mode_index: Resolution mode index
  */
 struct stimx274 {
struct v4l2_subdev sd;
@@ -567,7 +565,6 @@ struct stimx274 {
struct gpio_desc *reset_gpio;
struct mutex lock; /* mutex lock for operations */
const struct imx274_frmfmt *mode;
-   u32 mode_index;
 };
 
 /*
@@ -880,7 +877,6 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
index = 0;
}
 
-   imx274->mode_index = index;
imx274->mode = _formats[index];
 
if (fmt->width > IMX274_MAX_WIDTH)
@@ -1029,7 +1025,8 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
int ret = 0;
 
dev_dbg(>client->dev, "%s : %s, mode index = %d\n", __func__,
-   on ? "Stream Start" : "Stream Stop", imx274->mode_index);
+   on ? "Stream Start" : "Stream Stop",
+   imx274->mode - _formats[0]);
 
mutex_lock(>lock);
 
@@ -1068,8 +1065,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
}
 
mutex_unlock(>lock);
-   dev_dbg(>client->dev,
-   "%s : Done: mode = %d\n", __func__, imx274->mode_index);
+   dev_dbg(>client->dev, "%s : Done\n", __func__);
return 0;
 
 fail:
@@ -1625,8 +1621,7 @@ static int imx274_probe(struct i2c_client *client,
mutex_init(>lock);
 
/* initialize format */
-   imx274->mode_index = IMX274_MODE_3840X2160;
-   imx274->mode = _formats[imx274->mode_index];
+   imx274->mode = _formats[IMX274_MODE_3840X2160];
imx274->format.width = imx274->mode->size.width;
imx274->format.height = imx274->mode->size.height;
imx274->format.field = V4L2_FIELD_NONE;
-- 
2.7.4

[PATCH v2 10/13] media: imx274: actually use IMX274_DEFAULT_MODE

[Luca Ceresoli]

IMX274_DEFAULT_MODE is defined but not used. Start using it, so the
default can be more easily changed without digging into the code.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>

---
Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 69fdc82d4214..ae90fb31416c 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1621,7 +1621,7 @@ static int imx274_probe(struct i2c_client *client,
mutex_init(>lock);
 
/* initialize format */
-   imx274->mode = _formats[IMX274_MODE_3840X2160];
+   imx274->mode = _formats[IMX274_DEFAULT_MODE];
imx274->format.width = imx274->mode->size.width;
imx274->format.height = imx274->mode->size.height;
imx274->format.field = V4L2_FIELD_NONE;
-- 
2.7.4

[PATCH v2 07/13] media: imx274: initialize format before v4l2 controls

[Luca Ceresoli]

The current probe function calls v4l2_ctrl_handler_setup() before
initializing the format info. This triggers call paths such as:
imx274_probe -> v4l2_ctrl_handler_setup -> imx274_s_ctrl ->
imx274_set_exposure, where priv->mode_index is accessed before being
assigned.

This is wrong but does not trigger a visible bug because priv is
zero-initialized and 0 is the default value for priv->mode_index. But
this would become a crash in follow-up commits when mode_index is
replaced by a pointer that must always be valid.

Fix the bug before it shows up by initializing struct members early.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>

---
Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 63fb94e7da37..8a8a11b8d75d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1632,6 +1632,16 @@ static int imx274_probe(struct i2c_client *client,
 
mutex_init(>lock);
 
+   /* initialize format */
+   imx274->mode_index = IMX274_MODE_3840X2160;
+   imx274->format.width = imx274_formats[0].size.width;
+   imx274->format.height = imx274_formats[0].size.height;
+   imx274->format.field = V4L2_FIELD_NONE;
+   imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
+   imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
+   imx274->frame_interval.numerator = 1;
+   imx274->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
+
/* initialize regmap */
imx274->regmap = devm_regmap_init_i2c(client, _regmap_config);
if (IS_ERR(imx274->regmap)) {
@@ -1720,16 +1730,6 @@ static int imx274_probe(struct i2c_client *client,
goto err_ctrls;
}
 
-   /* initialize format */
-   imx274->mode_index = IMX274_MODE_3840X2160;
-   imx274->format.width = imx274_formats[0].size.width;
-   imx274->format.height = imx274_formats[0].size.height;
-   imx274->format.field = V4L2_FIELD_NONE;
-   imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
-   imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
-   imx274->frame_interval.numerator = 1;
-   imx274->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
-
/* load default control values */
ret = imx274_load_default(imx274);
if (ret) {
-- 
2.7.4

[PATCH v2 03/13] media: imx274: slightly simplify code

[Luca Ceresoli]

imx274_s_frame_interval() already has a direct pointer to the v4l2
exposure control, so reuse it to simplify code.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>

---
Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index dfd04edcdd48..c5d00ade4d64 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -984,7 +984,7 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
}
 
/* update exposure time accordingly */
-   imx274_set_exposure(imx274, imx274->ctrls.exposure->val);
+   imx274_set_exposure(imx274, ctrl->val);
 
dev_dbg(>client->dev, "set frame interval to %uus\n",
fi->interval.numerator * 100
-- 
2.7.4

[PATCH v2 02/13] media: imx274: fix typo in comment

[Luca Ceresoli]

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>

---
Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 5e425db9204d..dfd04edcdd48 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -971,7 +971,7 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
if (!ret) {
/*
 * exposure time range is decided by frame interval
-* need to update it after frame interal changes
+* need to update it after frame interval changes
 */
min = IMX274_MIN_EXPOSURE_TIME;
max = fi->interval.numerator * 100
-- 
2.7.4

[PATCH v2 01/13] media: imx274: document reset delays more clearly

[Luca Ceresoli]

Document the unit to avoid having to look through the code to compute it.
Also clarify that these are min and max values.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>

---
Changed v1 -> v2:
 - add "media: " prefix to commit message
---
 drivers/media/i2c/imx274.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index daec33f4196a..5e425db9204d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -87,7 +87,7 @@
 #define IMX274_SHR_LIMIT_CONST (4)
 
 /*
- * Constants for sensor reset delay
+ * Min and max sensor reset delay (microseconds)
  */
 #define IMX274_RESET_DELAY1(2000)
 #define IMX274_RESET_DELAY2(2200)
-- 
2.7.4

[PATCH 07/13] imx274: initialize format before v4l2 controls

[Luca Ceresoli]

The current probe function calls v4l2_ctrl_handler_setup() before
initializing the format info. This triggers call paths such as:
imx274_probe -> v4l2_ctrl_handler_setup -> imx274_s_ctrl ->
imx274_set_exposure, where priv->mode_index is accessed before being
assigned.

This is wrong but does not trigger a visible bug because priv is
zero-initialized and 0 is the default value for priv->mode_index. But
this would become a crash in follow-up commits when mode_index is
replaced by a pointer that must always be valid.

Fix the bug before it shows up by initializing struct members early.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 63fb94e7da37..8a8a11b8d75d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1632,6 +1632,16 @@ static int imx274_probe(struct i2c_client *client,
 
mutex_init(>lock);
 
+   /* initialize format */
+   imx274->mode_index = IMX274_MODE_3840X2160;
+   imx274->format.width = imx274_formats[0].size.width;
+   imx274->format.height = imx274_formats[0].size.height;
+   imx274->format.field = V4L2_FIELD_NONE;
+   imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
+   imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
+   imx274->frame_interval.numerator = 1;
+   imx274->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
+
/* initialize regmap */
imx274->regmap = devm_regmap_init_i2c(client, _regmap_config);
if (IS_ERR(imx274->regmap)) {
@@ -1720,16 +1730,6 @@ static int imx274_probe(struct i2c_client *client,
goto err_ctrls;
}
 
-   /* initialize format */
-   imx274->mode_index = IMX274_MODE_3840X2160;
-   imx274->format.width = imx274_formats[0].size.width;
-   imx274->format.height = imx274_formats[0].size.height;
-   imx274->format.field = V4L2_FIELD_NONE;
-   imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
-   imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
-   imx274->frame_interval.numerator = 1;
-   imx274->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
-
/* load default control values */
ret = imx274_load_default(imx274);
if (ret) {
-- 
2.7.4

[PATCH 08/13] imx274: consolidate per-mode data in imx274_frmfmt

[Luca Ceresoli]

Data about for the implemented readout modes is partially stored in
imx274_formats[], the rest is scattered in several arrays. The latter
are then accessed using the mode index, e.g.:

  min_frame_len[priv->mode_index]

Consolidate all these data in imx274_formats[], and store a pointer to
the selected mode (i.e. imx274_formats[priv->mode_index]) in the main
device struct. This way code to use these data becomes more readable,
e.g.:

  priv->mode->min_frame_len

This removes lots of scaffolding code and keeps data about each mode
in a unique place.

Also remove a parameter to imx274_mode_regs() that becomes unused.

While this adds the mode pointer to the device struct, it does not
remove the mode_index from it because mode_index is still used in two
dev_dbg() calls.  This will be handled in a follow-up commit.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 139 +
 1 file changed, 66 insertions(+), 73 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 8a8a11b8d75d..2ec31ae4e60d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -147,10 +147,28 @@ enum imx274_mode {
 };
 
 /*
- * imx274 format related structure
+ * Parameters for each imx274 readout mode.
+ *
+ * These are the values to configure the sensor in one of the
+ * implemented modes.
+ *
+ * @size: recommended recording pixels
+ * @init_regs: registers to initialize the mode
+ * @min_frame_len: Minimum frame length for each mode (see "Frame Rate
+ * Adjustment (CSI-2)" in the datasheet)
+ * @min_SHR: Minimum SHR register value (see "Shutter Setting (CSI-2)" in the
+ *   datasheet)
+ * @max_fps: Maximum frames per second
+ * @nocpiop: Number of clocks per internal offset period (see "Integration Time
+ *   in Each Readout Drive Mode (CSI-2)" in the datasheet)
  */
 struct imx274_frmfmt {
struct v4l2_frmsize_discrete size;
+   const struct reg_8 *init_regs;
+   int min_frame_len;
+   int min_SHR;
+   int max_fps;
+   int nocpiop;
 };
 
 /*
@@ -476,58 +494,35 @@ static const struct reg_8 imx274_tp_regs[] = {
{IMX274_TABLE_END, 0x00}
 };
 
-static const struct reg_8 *mode_table[] = {
-   [IMX274_MODE_3840X2160] = imx274_mode1_3840x2160_raw10,
-   [IMX274_MODE_1920X1080] = imx274_mode3_1920x1080_raw10,
-   [IMX274_MODE_1280X720]  = imx274_mode5_1280x720_raw10,
-};
-
-/*
- * imx274 format related structure
- */
+/* nocpiop happens to be the same number for the implemented modes */
 static const struct imx274_frmfmt imx274_formats[] = {
-   { {3840, 2160} },
-   { {1920, 1080} },
-   { {1280,  720} },
-};
-
-/*
- * minimal frame length for each mode
- * refer to datasheet section "Frame Rate Adjustment (CSI-2)"
- */
-static const int min_frame_len[] = {
-   4550, /* mode 1, 4K */
-   2310, /* mode 3, 1080p */
-   2310 /* mode 5, 720p */
-};
-
-/*
- * minimal numbers of SHR register
- * refer to datasheet table "Shutter Setting (CSI-2)"
- */
-static const int min_SHR[] = {
-   12, /* mode 1, 4K */
-   8, /* mode 3, 1080p */
-   8 /* mode 5, 720p */
-};
-
-static const int max_frame_rate[] = {
-   60, /* mode 1 , 4K */
-   120, /* mode 3, 1080p */
-   120 /* mode 5, 720p */
-};
-
-/*
- * Number of clocks per internal offset period
- * a constant based on mode
- * refer to section "Integration Time in Each Readout Drive Mode (CSI-2)"
- * in the datasheet
- * for the implemented 3 modes, it happens to be the same number
- */
-static const int nocpiop[] = {
-   112, /* mode 1 , 4K */
-   112, /* mode 3, 1080p */
-   112 /* mode 5, 720p */
+   {
+   /* mode 1, 4K */
+   .size = {3840, 2160},
+   .init_regs = imx274_mode1_3840x2160_raw10,
+   .min_frame_len = 4550,
+   .min_SHR = 12,
+   .max_fps = 60,
+   .nocpiop = 112,
+   },
+   {
+   /* mode 3, 1080p */
+   .size = {1920, 1080},
+   .init_regs = imx274_mode3_1920x1080_raw10,
+   .min_frame_len = 2310,
+   .min_SHR = 8,
+   .max_fps = 120,
+   .nocpiop = 112,
+   },
+   {
+   /* mode 5, 720p */
+   .size = {1280, 720},
+   .init_regs = imx274_mode5_1280x720_raw10,
+   .min_frame_len = 2310,
+   .min_SHR = 8,
+   .max_fps = 120,
+   .nocpiop = 112,
+   },
 };
 
 /*
@@ -557,6 +552,8 @@ struct imx274_ctrls {
  * @regmap: Pointer to regmap structure
  * @reset_gpio: Pointer to reset gpio
  * @lock: Mutex structure
+ * @mode: Parameters for the selected readout mode
+ *(points to imx274_formats[mode_index])
  * @mode_index: Resolution mode index
  */

[PATCH 10/13] imx274: actually use IMX274_DEFAULT_MODE

[Luca Ceresoli]

IMX274_DEFAULT_MODE is defined but not used. Start using it, so the
default can be more easily changed without digging into the code.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 25907d0817a4..39d548ce30c4 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1621,7 +1621,7 @@ static int imx274_probe(struct i2c_client *client,
mutex_init(>lock);
 
/* initialize format */
-   imx274->mode = _formats[IMX274_MODE_3840X2160];
+   imx274->mode = _formats[IMX274_DEFAULT_MODE];
imx274->format.width = imx274->mode->size.width;
imx274->format.height = imx274->mode->size.height;
imx274->format.field = V4L2_FIELD_NONE;
-- 
2.7.4

[PATCH 09/13] imx274: get rid of mode_index

[Luca Ceresoli]

After restructuring struct imx274_frmfmt, the mode_index field is
still in use only for two dev_dbg() calls in imx274_s_stream(). Let's
remove it and avoid duplicated information.

Replacing the first usage requires a rather annoying but trivial
computation. The other one can be removed entirely since it prints the
same value anyway.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 15 +--
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 2ec31ae4e60d..25907d0817a4 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -553,8 +553,6 @@ struct imx274_ctrls {
  * @reset_gpio: Pointer to reset gpio
  * @lock: Mutex structure
  * @mode: Parameters for the selected readout mode
- *(points to imx274_formats[mode_index])
- * @mode_index: Resolution mode index
  */
 struct stimx274 {
struct v4l2_subdev sd;
@@ -567,7 +565,6 @@ struct stimx274 {
struct gpio_desc *reset_gpio;
struct mutex lock; /* mutex lock for operations */
const struct imx274_frmfmt *mode;
-   u32 mode_index;
 };
 
 /*
@@ -880,7 +877,6 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
index = 0;
}
 
-   imx274->mode_index = index;
imx274->mode = _formats[index];
 
if (fmt->width > IMX274_MAX_WIDTH)
@@ -1028,8 +1024,9 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
struct stimx274 *imx274 = to_imx274(sd);
int ret = 0;
 
-   dev_dbg(>client->dev, "%s : %s, mode index = %d\n", __func__,
-   on ? "Stream Start" : "Stream Stop", imx274->mode_index);
+   dev_dbg(>client->dev, "%s : %s, mode index %lu\n", __func__,
+   on ? "Stream Start" : "Stream Stop",
+   imx274->mode - _formats[0]);
 
mutex_lock(>lock);
 
@@ -1068,8 +1065,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
}
 
mutex_unlock(>lock);
-   dev_dbg(>client->dev,
-   "%s : Done: mode = %d\n", __func__, imx274->mode_index);
+   dev_dbg(>client->dev, "%s : Done\n", __func__);
return 0;
 
 fail:
@@ -1625,8 +1621,7 @@ static int imx274_probe(struct i2c_client *client,
mutex_init(>lock);
 
/* initialize format */
-   imx274->mode_index = IMX274_MODE_3840X2160;
-   imx274->mode = _formats[imx274->mode_index];
+   imx274->mode = _formats[IMX274_MODE_3840X2160];
imx274->format.width = imx274->mode->size.width;
imx274->format.height = imx274->mode->size.height;
imx274->format.field = V4L2_FIELD_NONE;
-- 
2.7.4

[PATCH 01/13] imx274: document reset delays more clearly

[Luca Ceresoli]

Document the unit to avoid having to look through the code to compute it.
Also clarify that these are min and max values.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index daec33f4196a..5e425db9204d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -87,7 +87,7 @@
 #define IMX274_SHR_LIMIT_CONST (4)
 
 /*
- * Constants for sensor reset delay
+ * Min and max sensor reset delay (microseconds)
  */
 #define IMX274_RESET_DELAY1(2000)
 #define IMX274_RESET_DELAY2(2200)
-- 
2.7.4

[PATCH 11/13] imx274: simplify imx274_write_table()

[Luca Ceresoli]

imx274_write_table() is a mere wrapper (and the only user) to
imx274_regmap_util_write_table_8(). Remove this useless indirection by
merging the two functions into one.

Also get rid of the wait_ms_addr and end_addr parameters since it does
not make any sense to give them any values other than
IMX274_TABLE_WAIT_MS and IMX274_TABLE_END.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 28 ++--
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 39d548ce30c4..ebe37a087a02 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -597,20 +597,18 @@ static inline struct stimx274 *to_imx274(struct 
v4l2_subdev *sd)
 }
 
 /*
- * imx274_regmap_util_write_table_8 - Function for writing register table
- * @regmap: Pointer to device reg map structure
- * @table: Table containing register values
- * @wait_ms_addr: Flag for performing delay
- * @end_addr: Flag for incating end of table
+ * Writing a register table
+ *
+ * @priv: Pointer to device
+ * @table: Table containing register values (with optional delays)
  *
  * This is used to write register table into sensor's reg map.
  *
  * Return: 0 on success, errors otherwise
  */
-static int imx274_regmap_util_write_table_8(struct regmap *regmap,
-   const struct reg_8 table[],
-   u16 wait_ms_addr, u16 end_addr)
+static int imx274_write_table(struct stimx274 *priv, const struct reg_8 
table[])
 {
+   struct regmap *regmap = priv->regmap;
int err = 0;
const struct reg_8 *next;
u8 val;
@@ -622,8 +620,8 @@ static int imx274_regmap_util_write_table_8(struct regmap 
*regmap,
 
for (next = table;; next++) {
if ((next->addr != range_start + range_count) ||
-   (next->addr == end_addr) ||
-   (next->addr == wait_ms_addr) ||
+   (next->addr == IMX274_TABLE_END) ||
+   (next->addr == IMX274_TABLE_WAIT_MS) ||
(range_count == max_range_vals)) {
if (range_count == 1)
err = regmap_write(regmap,
@@ -642,10 +640,10 @@ static int imx274_regmap_util_write_table_8(struct regmap 
*regmap,
range_count = 0;
 
/* Handle special address values */
-   if (next->addr == end_addr)
+   if (next->addr == IMX274_TABLE_END)
break;
 
-   if (next->addr == wait_ms_addr) {
+   if (next->addr == IMX274_TABLE_WAIT_MS) {
msleep_range(next->val);
continue;
}
@@ -692,12 +690,6 @@ static inline int imx274_write_reg(struct stimx274 *priv, 
u16 addr, u8 val)
return err;
 }
 
-static int imx274_write_table(struct stimx274 *priv, const struct reg_8 
table[])
-{
-   return imx274_regmap_util_write_table_8(priv->regmap,
-   table, IMX274_TABLE_WAIT_MS, IMX274_TABLE_END);
-}
-
 /*
  * Set mode registers to start stream.
  * @priv: Pointer to device structure
-- 
2.7.4

[PATCH 12/13] imx274: add helper function to fill a reg_8 table chunk

[Luca Ceresoli]

Tables of struct reg_8 are used to simplify multi-byte register
assignment. However filling these snippets with values computed at
runtime is currently implemented by very similar functions doing the
needed shift & mask manipulation.

Replace all those functions with a unique helper function to fill
reg_8 tables in a simple and clean way.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 90 --
 1 file changed, 55 insertions(+), 35 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index ebe37a087a02..0b3183afdd7b 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -597,6 +597,58 @@ static inline struct stimx274 *to_imx274(struct 
v4l2_subdev *sd)
 }
 
 /*
+ * Fill consecutive reg_8 items in order to set a multibyte register.
+ *
+ * The sensor has many 2-bytes registers and a 3-byte register. This
+ * simplifies code to set them by filling consecutive entries of a
+ * struct reg_8 table with the data to set a register.
+ *
+ * The number of table entries that is filled is the minimum needed
+ * for the given number of bits (i.e. nbits / 8, rounded up). If nbits
+ * is not a multiple of 8, extra bits in the most significant byte are
+ * zeroed.
+ *
+ * Example:
+ *   Calling prepare_reg([10], 0x3000, 0xcafe, 12) will set:
+ *   regs[10] = { .addr = 0x3000, .val = 0xfe }
+ *   regs[11] = { .addr = 0x3001, .val = 0x0a }
+ *
+ * @table_base: Pointer to the first reg_8 struct to be filled.  The
+ *  following entries will also be set, make sure they are
+ *  properly allocated!
+ * @addr_lsb: Address of the LSB register.  Other registers must be
+ * consecutive, least-to-most significant.
+ * @value: Value to be written to the register.
+ * @nbits: Number of bits to write (range: [1..24])
+ */
+static void prepare_reg(struct reg_8 *table_base,
+   u16 addr_lsb,
+   u32 value,
+   size_t nbits)
+{
+   struct reg_8 *cmd = table_base;
+   u16 addr = addr_lsb;
+   size_t bits; /* how many bits at this round */
+
+   WARN_ON(nbits > 24);
+
+   if (nbits > 24)
+   nbits = 24;
+
+   while (nbits > 0) {
+   bits = min_t(size_t, 8, nbits);
+
+   cmd->addr = addr;
+   cmd->val = value & ((1 << bits) - 1);
+
+   nbits -= bits;
+   value >>= 8;
+   addr++;
+   cmd++;
+   }
+}
+
+/*
  * Writing a register table
  *
  * @priv: Pointer to device
@@ -1163,15 +1215,6 @@ static int imx274_set_digital_gain(struct stimx274 
*priv, u32 dgain)
reg_val & IMX274_MASK_LSB_4_BITS);
 }
 
-static inline void imx274_calculate_gain_regs(struct reg_8 regs[2], u16 gain)
-{
-   regs->addr = IMX274_ANALOG_GAIN_ADDR_MSB;
-   regs->val = (gain >> IMX274_SHIFT_8_BITS) & IMX274_MASK_LSB_3_BITS;
-
-   (regs + 1)->addr = IMX274_ANALOG_GAIN_ADDR_LSB;
-   (regs + 1)->val = (gain) & IMX274_MASK_LSB_8_BITS;
-}
-
 /*
  * imx274_set_gain - Function called when setting gain
  * @priv: Pointer to device structure
@@ -1229,7 +1272,7 @@ static int imx274_set_gain(struct stimx274 *priv, struct 
v4l2_ctrl *ctrl)
if (gain_reg > IMX274_GAIN_REG_MAX)
gain_reg = IMX274_GAIN_REG_MAX;
 
-   imx274_calculate_gain_regs(reg_list, (u16)gain_reg);
+   prepare_reg(reg_list, IMX274_ANALOG_GAIN_ADDR_LSB, gain_reg, 11);
 
for (i = 0; i < ARRAY_SIZE(reg_list); i++) {
err = imx274_write_reg(priv, reg_list[i].addr,
@@ -1258,16 +1301,6 @@ static int imx274_set_gain(struct stimx274 *priv, struct 
v4l2_ctrl *ctrl)
return err;
 }
 
-static inline void imx274_calculate_coarse_time_regs(struct reg_8 regs[2],
-u32 coarse_time)
-{
-   regs->addr = IMX274_SHR_REG_MSB;
-   regs->val = (coarse_time >> IMX274_SHIFT_8_BITS)
-   & IMX274_MASK_LSB_8_BITS;
-   (regs + 1)->addr = IMX274_SHR_REG_LSB;
-   (regs + 1)->val = (coarse_time) & IMX274_MASK_LSB_8_BITS;
-}
-
 /*
  * imx274_set_coarse_time - Function called when setting SHR value
  * @priv: Pointer to device structure
@@ -1292,7 +1325,7 @@ static int imx274_set_coarse_time(struct stimx274 *priv, 
u32 *val)
goto fail;
 
/* prepare SHR registers */
-   imx274_calculate_coarse_time_regs(reg_list, coarse_time);
+   prepare_reg(reg_list, IMX274_SHR_REG_LSB, coarse_time, 16);
 
/* write to SHR registers */
for (i = 0; i < ARRAY_SIZE(reg_list); i++) {
@@ -1429,19 +1462,6 @@ static int imx274_set_test_pattern(struct stimx274 
*priv, int val)
return err;
 }
 
-static inline void imx274_calculate_frame_length_regs(struct reg_8 regs[3],
-  

[PATCH 13/13] imx274: add SELECTION support for cropping

[Luca Ceresoli]

Currently this driver does not support cropping. The supported modes
are the following, all capturing the entire area:

 - 3840x2160, 1:1 binning (native sensor resolution)
 - 1920x1080, 2:1 binning
 - 1280x720,  3:1 binning

The set_fmt callback chooses among these 3 configurations the one that
matches the requested format.

Adding support to VIDIOC_SUBDEV_G_SELECTION and
VIDIOC_SUBDEV_S_SELECTION involved a complete rewrite of set_fmt,
which now chooses the most appropriate binning based on the ratio
between the previously-set cropping size and the format size being
requested.

Note that the names in enum imx274_mode change from being
resolution-based to being binning-based. Without cropping, the two
naming are equivalent. With cropping, the resolution could be
different, e.g. using 2:1 binning mode to crop 1200x960 and output a
600x480 format. Using binning in the names avoids anny
misunderstanding.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 266 -
 1 file changed, 192 insertions(+), 74 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 0b3183afdd7b..db843e0ce057 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -19,6 +19,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include 
 #include 
 #include 
 #include 
@@ -74,7 +75,7 @@
  */
 #define IMX274_MIN_EXPOSURE_TIME   (4 * 260 / 72)
 
-#define IMX274_DEFAULT_MODEIMX274_MODE_3840X2160
+#define IMX274_DEFAULT_MODEIMX274_MODE_BINNING_OFF
 #define IMX274_MAX_WIDTH   (3840)
 #define IMX274_MAX_HEIGHT  (2160)
 #define IMX274_MAX_FRAME_RATE  (120)
@@ -111,6 +112,20 @@
 #define IMX274_SHR_REG_LSB 0x300C /* SHR */
 #define IMX274_SVR_REG_MSB 0x300F /* SVR */
 #define IMX274_SVR_REG_LSB 0x300E /* SVR */
+#define IMX274_HTRIM_EN_REG0x3037
+#define IMX274_HTRIM_START_REG_LSB 0x3038
+#define IMX274_HTRIM_START_REG_MSB 0x3039
+#define IMX274_HTRIM_END_REG_LSB   0x303A
+#define IMX274_HTRIM_END_REG_MSB   0x303B
+#define IMX274_VWIDCUTEN_REG   0x30DD
+#define IMX274_VWIDCUT_REG_LSB 0x30DE
+#define IMX274_VWIDCUT_REG_MSB 0x30DF
+#define IMX274_VWINPOS_REG_LSB 0x30E0
+#define IMX274_VWINPOS_REG_MSB 0x30E1
+#define IMX274_WRITE_VSIZE_REG_LSB 0x3130
+#define IMX274_WRITE_VSIZE_REG_MSB 0x3131
+#define IMX274_Y_OUT_SIZE_REG_LSB  0x3132
+#define IMX274_Y_OUT_SIZE_REG_MSB  0x3133
 #define IMX274_VMAX_REG_1  0x30FA /* VMAX, MSB */
 #define IMX274_VMAX_REG_2  0x30F9 /* VMAX */
 #define IMX274_VMAX_REG_3  0x30F8 /* VMAX, LSB */
@@ -141,9 +156,9 @@ static const struct regmap_config imx274_regmap_config = {
 };
 
 enum imx274_mode {
-   IMX274_MODE_3840X2160,
-   IMX274_MODE_1920X1080,
-   IMX274_MODE_1280X720,
+   IMX274_MODE_BINNING_OFF,
+   IMX274_MODE_BINNING_2_1,
+   IMX274_MODE_BINNING_3_1,
 };
 
 /*
@@ -215,31 +230,14 @@ static const struct reg_8 imx274_mode1_3840x2160_raw10[] 
= {
{0x3004, 0x01},
{0x3005, 0x01},
{0x3006, 0x00},
-   {0x3007, 0x02},
+   {0x3007, 0xa2},
 
{0x3018, 0xA2}, /* output XVS, HVS */
 
{0x306B, 0x05},
{0x30E2, 0x01},
-   {0x30F6, 0x07}, /* HMAX, 263 */
-   {0x30F7, 0x01}, /* HMAX */
-
-   {0x30dd, 0x01}, /* crop to 2160 */
-   {0x30de, 0x06},
-   {0x30df, 0x00},
-   {0x30e0, 0x12},
-   {0x30e1, 0x00},
-   {0x3037, 0x01}, /* to crop to 3840 */
-   {0x3038, 0x0c},
-   {0x3039, 0x00},
-   {0x303a, 0x0c},
-   {0x303b, 0x0f},
 
{0x30EE, 0x01},
-   {0x3130, 0x86},
-   {0x3131, 0x08},
-   {0x3132, 0x7E},
-   {0x3133, 0x08},
{0x3342, 0x0A},
{0x3343, 0x00},
{0x3344, 0x16},
@@ -273,7 +271,7 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] = {
{0x3004, 0x02},
{0x3005, 0x21},
{0x3006, 0x00},
-   {0x3007, 0x11},
+   {0x3007, 0xb1},
 
{0x3018, 0xA2}, /* output XVS, HVS */
 
@@ -283,22 +281,7 @@ static const struct reg_8 imx274_mode3_1920x1080_raw10[] = 
{
{0x30F6, 0x04}, /* HMAX, 260 */
{0x30F7, 0x01}, /* HMAX */
 
-   {0x30dd, 0x01}, /* to crop to 1920x1080 */
-   {0x30de, 0x05},
-   {0x30df, 0x00},
-   {0x30e0, 0x04},
-   {0x30e1, 0x00},
-   {0x3037, 0x01},
-   {0x3038, 0x0c},
-   {0x3039, 0x00},
-   {0x303a, 0x0c},
-   {0x303b, 0x0f},
-
{0x30EE, 0x01},
-   {0x3130, 0x4E},
-   {0x3131, 0x04},
-   {0x3132, 0x46},
-   {0x3133, 0x04},
{

[PATCH 06/13] imx274: remove non-indexed pointers from mode_table

[Luca Ceresoli]

mode_table[] has 3 members that are accessed based on their index, which
makes worth using an array.

The other members are always accessed with a constant index. This added
indirection gives no improvement and only makes code more verbose.

Remove these pointers from the array and access them directly.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 25 ++---
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 62a0d7af0e51..63fb94e7da37 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -144,12 +144,6 @@ enum imx274_mode {
IMX274_MODE_3840X2160,
IMX274_MODE_1920X1080,
IMX274_MODE_1280X720,
-
-   IMX274_MODE_START_STREAM_1,
-   IMX274_MODE_START_STREAM_2,
-   IMX274_MODE_START_STREAM_3,
-   IMX274_MODE_START_STREAM_4,
-   IMX274_MODE_STOP_STREAM
 };
 
 /*
@@ -486,12 +480,6 @@ static const struct reg_8 *mode_table[] = {
[IMX274_MODE_3840X2160] = imx274_mode1_3840x2160_raw10,
[IMX274_MODE_1920X1080] = imx274_mode3_1920x1080_raw10,
[IMX274_MODE_1280X720]  = imx274_mode5_1280x720_raw10,
-
-   [IMX274_MODE_START_STREAM_1]= imx274_start_1,
-   [IMX274_MODE_START_STREAM_2]= imx274_start_2,
-   [IMX274_MODE_START_STREAM_3]= imx274_start_3,
-   [IMX274_MODE_START_STREAM_4]= imx274_start_4,
-   [IMX274_MODE_STOP_STREAM]   = imx274_stop,
 };
 
 /*
@@ -731,11 +719,11 @@ static int imx274_mode_regs(struct stimx274 *priv, int 
mode)
 {
int err = 0;
 
-   err = imx274_write_table(priv, mode_table[IMX274_MODE_START_STREAM_1]);
+   err = imx274_write_table(priv, imx274_start_1);
if (err)
return err;
 
-   err = imx274_write_table(priv, mode_table[IMX274_MODE_START_STREAM_2]);
+   err = imx274_write_table(priv, imx274_start_2);
if (err)
return err;
 
@@ -760,7 +748,7 @@ static int imx274_start_stream(struct stimx274 *priv)
 * give it 1 extra ms for margin
 */
msleep_range(11);
-   err = imx274_write_table(priv, mode_table[IMX274_MODE_START_STREAM_3]);
+   err = imx274_write_table(priv, imx274_start_3);
if (err)
return err;
 
@@ -770,7 +758,7 @@ static int imx274_start_stream(struct stimx274 *priv)
 * give it 1 extra ms for margin
 */
msleep_range(8);
-   err = imx274_write_table(priv, mode_table[IMX274_MODE_START_STREAM_4]);
+   err = imx274_write_table(priv, imx274_start_4);
if (err)
return err;
 
@@ -1081,8 +1069,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
goto fail;
} else {
/* stop stream */
-   ret = imx274_write_table(imx274,
-mode_table[IMX274_MODE_STOP_STREAM]);
+   ret = imx274_write_table(imx274, imx274_stop);
if (ret)
goto fail;
}
@@ -1779,7 +1766,7 @@ static int imx274_remove(struct i2c_client *client)
struct stimx274 *imx274 = to_imx274(sd);
 
/* stop stream */
-   imx274_write_table(imx274, mode_table[IMX274_MODE_STOP_STREAM]);
+   imx274_write_table(imx274, imx274_stop);
 
v4l2_async_unregister_subdev(sd);
v4l2_ctrl_handler_free(>ctrls.handler);
-- 
2.7.4

[PATCH 00/13] imx274: add cropping and misc improvements

[Luca Ceresoli]

Hi,

this patchset introduces cropping support for the Sony IMX274 sensor
using the SELECTION API, after several cleanups and general code
improvements.

Patches 1-6 clean up and restructure code in various places and are
pretty much independent from the cropping feature.

Patches 7-11 are further restructuring which are mostly useful to
implement cropping API in a cleaner way.

Patch 12 introduces a helper to allow setting many registers computed
at runtime in a straightforward way. This would not have been very
useful before because all long register write sequences came from
const tables, but it's definitely a must for the cropping code.

Patch 13 implements cropping in the set_selection pad operation. On
the v4l2 side there is nothing special, it fit nicely in the existing
infrastructure. The most tricky part was respecting all the device
constraints on the horizontal crop.

Regards,
Luca

Luca Ceresoli (13):
  imx274: document reset delays more clearly
  imx274: fix typo in comment
  imx274: slightly simplify code
  imx274: remove unused data from struct imx274_frmfmt
  imx274: rename and reorder register address definitions
  imx274: remove non-indexed pointers from mode_table
  imx274: initialize format before v4l2 controls
  imx274: consolidate per-mode data in imx274_frmfmt
  imx274: get rid of mode_index
  imx274: actually use IMX274_DEFAULT_MODE
  imx274: simplify imx274_write_table()
  imx274: add helper function to fill a reg_8 table chunk
  imx274: add SELECTION support for cropping

 drivers/media/i2c/imx274.c | 600 ++---
 1 file changed, 349 insertions(+), 251 deletions(-)

-- 
2.7.4

[PATCH 03/13] imx274: slightly simplify code

[Luca Ceresoli]

imx274_s_frame_interval() already has a direct pointer to the v4l2
exposure control, so reuse it to simplify code.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index dfd04edcdd48..c5d00ade4d64 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -984,7 +984,7 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
}
 
/* update exposure time accordingly */
-   imx274_set_exposure(imx274, imx274->ctrls.exposure->val);
+   imx274_set_exposure(imx274, ctrl->val);
 
dev_dbg(>client->dev, "set frame interval to %uus\n",
fi->interval.numerator * 100
-- 
2.7.4

[PATCH 04/13] imx274: remove unused data from struct imx274_frmfmt

[Luca Ceresoli]

struct imx274_frmfmt is instantiated only in the imx274_formats[]
array, where imx274_formats[N].mode always equals N (via enum
imx274_mode).  So .mode carries no information, and unsurprisingly it
is never used.

mbus_code is never used because the 12 bit modes are not implemented.

The colorspace member is also never used, which is normal since the
imx274 sensor can output only one colorspace.

Let's get rid of all of them.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 17 +
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index c5d00ade4d64..9203d377cfe2 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -156,10 +156,7 @@ enum imx274_mode {
  * imx274 format related structure
  */
 struct imx274_frmfmt {
-   u32 mbus_code;
-   enum v4l2_colorspace colorspace;
struct v4l2_frmsize_discrete size;
-   enum imx274_mode mode;
 };
 
 /*
@@ -501,12 +498,9 @@ static const struct reg_8 *mode_table[] = {
  * imx274 format related structure
  */
 static const struct imx274_frmfmt imx274_formats[] = {
-   {MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB, {3840, 2160},
-   IMX274_MODE_3840X2160},
-   {MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB, {1920, 1080},
-   IMX274_MODE_1920X1080},
-   {MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB, {1280, 720},
-   IMX274_MODE_1280X720},
+   { {3840, 2160} },
+   { {1920, 1080} },
+   { {1280,  720} },
 };
 
 /*
@@ -890,9 +884,8 @@ static int imx274_set_fmt(struct v4l2_subdev *sd,
int index;
 
dev_dbg(>dev,
-   "%s: width = %d height = %d code = %d mbus_code = %d\n",
-   __func__, fmt->width, fmt->height, fmt->code,
-   imx274_formats[imx274->mode_index].mbus_code);
+   "%s: width = %d height = %d code = %d\n",
+   __func__, fmt->width, fmt->height, fmt->code);
 
mutex_lock(>lock);
 
-- 
2.7.4

[PATCH 02/13] imx274: fix typo in comment

[Luca Ceresoli]

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 5e425db9204d..dfd04edcdd48 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -971,7 +971,7 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
if (!ret) {
/*
 * exposure time range is decided by frame interval
-* need to update it after frame interal changes
+* need to update it after frame interval changes
 */
min = IMX274_MIN_EXPOSURE_TIME;
max = fi->interval.numerator * 100
-- 
2.7.4

[PATCH 05/13] imx274: rename and reorder register address definitions

[Luca Ceresoli]

Most registers are defined using the name used in the datasheet.
E.g. the defines for the HMAX register are IMX274_HMAX_REG_*.

Rename the SHR and VMAX register accordingly. Also move them close to
related registers: SHR close to SVR, VMAX close to HMAX.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 26 +-
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 9203d377cfe2..62a0d7af0e51 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -107,15 +107,15 @@
 /*
  * IMX274 register definitions
  */
-#define IMX274_FRAME_LENGTH_ADDR_1 0x30FA /* VMAX, MSB */
-#define IMX274_FRAME_LENGTH_ADDR_2 0x30F9 /* VMAX */
-#define IMX274_FRAME_LENGTH_ADDR_3 0x30F8 /* VMAX, LSB */
+#define IMX274_SHR_REG_MSB 0x300D /* SHR */
+#define IMX274_SHR_REG_LSB 0x300C /* SHR */
 #define IMX274_SVR_REG_MSB 0x300F /* SVR */
 #define IMX274_SVR_REG_LSB 0x300E /* SVR */
+#define IMX274_VMAX_REG_1  0x30FA /* VMAX, MSB */
+#define IMX274_VMAX_REG_2  0x30F9 /* VMAX */
+#define IMX274_VMAX_REG_3  0x30F8 /* VMAX, LSB */
 #define IMX274_HMAX_REG_MSB0x30F7 /* HMAX */
 #define IMX274_HMAX_REG_LSB0x30F6 /* HMAX */
-#define IMX274_COARSE_TIME_ADDR_MSB0x300D /* SHR */
-#define IMX274_COARSE_TIME_ADDR_LSB0x300C /* SHR */
 #define IMX274_ANALOG_GAIN_ADDR_LSB0x300A /* ANALOG GAIN LSB */
 #define IMX274_ANALOG_GAIN_ADDR_MSB0x300B /* ANALOG GAIN MSB */
 #define IMX274_DIGITAL_GAIN_REG0x3012 /* Digital Gain 
*/
@@ -1126,15 +1126,15 @@ static int imx274_get_frame_length(struct stimx274 
*priv, u32 *val)
svr = (reg_val[1] << IMX274_SHIFT_8_BITS) + reg_val[0];
 
/* vmax */
-   err = imx274_read_reg(priv, IMX274_FRAME_LENGTH_ADDR_3, _val[0]);
+   err = imx274_read_reg(priv, IMX274_VMAX_REG_3, _val[0]);
if (err)
goto fail;
 
-   err = imx274_read_reg(priv, IMX274_FRAME_LENGTH_ADDR_2, _val[1]);
+   err = imx274_read_reg(priv, IMX274_VMAX_REG_2, _val[1]);
if (err)
goto fail;
 
-   err = imx274_read_reg(priv, IMX274_FRAME_LENGTH_ADDR_1, _val[2]);
+   err = imx274_read_reg(priv, IMX274_VMAX_REG_1, _val[2]);
if (err)
goto fail;
 
@@ -1293,10 +1293,10 @@ static int imx274_set_gain(struct stimx274 *priv, 
struct v4l2_ctrl *ctrl)
 static inline void imx274_calculate_coarse_time_regs(struct reg_8 regs[2],
 u32 coarse_time)
 {
-   regs->addr = IMX274_COARSE_TIME_ADDR_MSB;
+   regs->addr = IMX274_SHR_REG_MSB;
regs->val = (coarse_time >> IMX274_SHIFT_8_BITS)
& IMX274_MASK_LSB_8_BITS;
-   (regs + 1)->addr = IMX274_COARSE_TIME_ADDR_LSB;
+   (regs + 1)->addr = IMX274_SHR_REG_LSB;
(regs + 1)->val = (coarse_time) & IMX274_MASK_LSB_8_BITS;
 }
 
@@ -1464,13 +1464,13 @@ static int imx274_set_test_pattern(struct stimx274 
*priv, int val)
 static inline void imx274_calculate_frame_length_regs(struct reg_8 regs[3],
  u32 frame_length)
 {
-   regs->addr = IMX274_FRAME_LENGTH_ADDR_1;
+   regs->addr = IMX274_VMAX_REG_1;
regs->val = (frame_length >> IMX274_SHIFT_16_BITS)
& IMX274_MASK_LSB_4_BITS;
-   (regs + 1)->addr = IMX274_FRAME_LENGTH_ADDR_2;
+   (regs + 1)->addr = IMX274_VMAX_REG_2;
(regs + 1)->val = (frame_length >> IMX274_SHIFT_8_BITS)
& IMX274_MASK_LSB_8_BITS;
-   (regs + 2)->addr = IMX274_FRAME_LENGTH_ADDR_3;
+   (regs + 2)->addr = IMX274_VMAX_REG_3;
(regs + 2)->val = (frame_length) & IMX274_MASK_LSB_8_BITS;
 }
 
-- 
2.7.4

[PATCH 2/5] media: docs: clarify relationship between crop and selection APIs

[Luca Ceresoli]

Having two somewhat similar and largely overlapping APIs is confusing,
especially since the older one appears in the docs before the newer
and most featureful counterpart.

Clarify all of this in several ways:
 - swap the two sections
 - give a name to the two APIs in the section names
 - add a note at the beginning of the CROP API section

Also remove a note that is incorrect (correct wording is in
vidioc-cropcap.rst).

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
Based on info from: Hans Verkuil <hverk...@xs4all.nl>
Cc: Hans Verkuil <hverk...@xs4all.nl>
---
 Documentation/media/uapi/v4l/common.rst|  2 +-
 Documentation/media/uapi/v4l/crop.rst  | 21 -
 Documentation/media/uapi/v4l/selection-api-005.rst |  2 ++
 Documentation/media/uapi/v4l/selection-api.rst |  4 ++--
 4 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/Documentation/media/uapi/v4l/common.rst 
b/Documentation/media/uapi/v4l/common.rst
index 13f2ed3fc5a6..5f93e71122ef 100644
--- a/Documentation/media/uapi/v4l/common.rst
+++ b/Documentation/media/uapi/v4l/common.rst
@@ -41,6 +41,6 @@ applicable to all devices.
 extended-controls
 format
 planar-apis
-crop
 selection-api
+crop
 streaming-par
diff --git a/Documentation/media/uapi/v4l/crop.rst 
b/Documentation/media/uapi/v4l/crop.rst
index 182565b9ace4..83fa16eb347e 100644
--- a/Documentation/media/uapi/v4l/crop.rst
+++ b/Documentation/media/uapi/v4l/crop.rst
@@ -2,9 +2,18 @@
 
 .. _crop:
 
-*
-Image Cropping, Insertion and Scaling
-*
+*
+Image Cropping, Insertion and Scaling -- the CROP API
+*
+
+.. note::
+
+   The CROP API is mostly superseded by the newer :ref:`SELECTION API
+   `. The new API should be preferred in most cases,
+   with the exception of pixel aspect ratio detection, which is
+   implemented by :ref:`VIDIOC_CROPCAP ` and has no
+   equivalent in the SELECTION API. See :ref:`selection-vs-crop` for a
+   comparison of the two APIs.
 
 Some video capture devices can sample a subsection of the picture and
 shrink or enlarge it to an image of arbitrary size. We call these
@@ -40,12 +49,6 @@ support scaling or the :ref:`VIDIOC_G_CROP ` 
and
 :ref:`VIDIOC_S_CROP ` ioctls. Their size (and position
 where applicable) will be fixed in this case.
 
-.. note::
-
-   All capture and output devices must support the
-   :ref:`VIDIOC_CROPCAP ` ioctl such that applications
-   can determine if scaling takes place.
-
 
 Cropping Structures
 ===
diff --git a/Documentation/media/uapi/v4l/selection-api-005.rst 
b/Documentation/media/uapi/v4l/selection-api-005.rst
index 5b47a28ac6d7..2ad30a49184f 100644
--- a/Documentation/media/uapi/v4l/selection-api-005.rst
+++ b/Documentation/media/uapi/v4l/selection-api-005.rst
@@ -1,5 +1,7 @@
 .. -*- coding: utf-8; mode: rst -*-
 
+.. _selection-vs-crop:
+
 
 Comparison with old cropping API
 
diff --git a/Documentation/media/uapi/v4l/selection-api.rst 
b/Documentation/media/uapi/v4l/selection-api.rst
index 81ea52d785b9..e4e623824b30 100644
--- a/Documentation/media/uapi/v4l/selection-api.rst
+++ b/Documentation/media/uapi/v4l/selection-api.rst
@@ -2,8 +2,8 @@
 
 .. _selection-api:
 
-API for cropping, composing and scaling
-===
+Cropping, composing and scaling -- the SELECTION API
+
 
 
 .. toctree::
-- 
2.7.4

[PATCH 3/5] media: docs: selection: rename files to something meaningful

[Luca Ceresoli]

These files have an automatically-generated numbering. Replaname them
to something that suggests their meaning.

Reported-by: Hans Verkuil <hverk...@xs4all.nl>
Cc: Hans Verkuil <hverk...@xs4all.nl>
Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
---
 .../{selection-api-004.rst => selection-api-configuration.rst} |  0
 .../v4l/{selection-api-006.rst => selection-api-examples.rst}  |  0
 .../v4l/{selection-api-002.rst => selection-api-intro.rst} |  0
 .../v4l/{selection-api-003.rst => selection-api-targets.rst}   |  0
 .../{selection-api-005.rst => selection-api-vs-crop-api.rst}   |  0
 Documentation/media/uapi/v4l/selection-api.rst | 10 +-
 6 files changed, 5 insertions(+), 5 deletions(-)
 rename Documentation/media/uapi/v4l/{selection-api-004.rst => 
selection-api-configuration.rst} (100%)
 rename Documentation/media/uapi/v4l/{selection-api-006.rst => 
selection-api-examples.rst} (100%)
 rename Documentation/media/uapi/v4l/{selection-api-002.rst => 
selection-api-intro.rst} (100%)
 rename Documentation/media/uapi/v4l/{selection-api-003.rst => 
selection-api-targets.rst} (100%)
 rename Documentation/media/uapi/v4l/{selection-api-005.rst => 
selection-api-vs-crop-api.rst} (100%)

diff --git a/Documentation/media/uapi/v4l/selection-api-004.rst 
b/Documentation/media/uapi/v4l/selection-api-configuration.rst
similarity index 100%
rename from Documentation/media/uapi/v4l/selection-api-004.rst
rename to Documentation/media/uapi/v4l/selection-api-configuration.rst
diff --git a/Documentation/media/uapi/v4l/selection-api-006.rst 
b/Documentation/media/uapi/v4l/selection-api-examples.rst
similarity index 100%
rename from Documentation/media/uapi/v4l/selection-api-006.rst
rename to Documentation/media/uapi/v4l/selection-api-examples.rst
diff --git a/Documentation/media/uapi/v4l/selection-api-002.rst 
b/Documentation/media/uapi/v4l/selection-api-intro.rst
similarity index 100%
rename from Documentation/media/uapi/v4l/selection-api-002.rst
rename to Documentation/media/uapi/v4l/selection-api-intro.rst
diff --git a/Documentation/media/uapi/v4l/selection-api-003.rst 
b/Documentation/media/uapi/v4l/selection-api-targets.rst
similarity index 100%
rename from Documentation/media/uapi/v4l/selection-api-003.rst
rename to Documentation/media/uapi/v4l/selection-api-targets.rst
diff --git a/Documentation/media/uapi/v4l/selection-api-005.rst 
b/Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst
similarity index 100%
rename from Documentation/media/uapi/v4l/selection-api-005.rst
rename to Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst
diff --git a/Documentation/media/uapi/v4l/selection-api.rst 
b/Documentation/media/uapi/v4l/selection-api.rst
index e4e623824b30..390233f704a3 100644
--- a/Documentation/media/uapi/v4l/selection-api.rst
+++ b/Documentation/media/uapi/v4l/selection-api.rst
@@ -9,8 +9,8 @@ Cropping, composing and scaling -- the SELECTION API
 .. toctree::
 :maxdepth: 1
 
-selection-api-002
-selection-api-003
-selection-api-004
-selection-api-005
-selection-api-006
+selection-api-intro.rst
+selection-api-targets.rst
+selection-api-configuration.rst
+selection-api-vs-crop-api.rst
+selection-api-examples.rst
-- 
2.7.4

[PATCH 1/5] media: docs: selection: fix typos

[Luca Ceresoli]

Cc: Hans Verkuil <hverk...@xs4all.nl>
Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
---
 Documentation/media/uapi/v4l/selection-api-004.rst | 2 +-
 Documentation/media/uapi/v4l/selection.svg | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/media/uapi/v4l/selection-api-004.rst 
b/Documentation/media/uapi/v4l/selection-api-004.rst
index d782cd5b2117..0a4ddc2d71db 100644
--- a/Documentation/media/uapi/v4l/selection-api-004.rst
+++ b/Documentation/media/uapi/v4l/selection-api-004.rst
@@ -41,7 +41,7 @@ The driver may further adjust the requested size and/or 
position
 according to hardware limitations.
 
 Each capture device has a default source rectangle, given by the
-``V4L2_SEL_TGT_CROP_DEFAULT`` target. This rectangle shall over what the
+``V4L2_SEL_TGT_CROP_DEFAULT`` target. This rectangle shall cover what the
 driver writer considers the complete picture. Drivers shall set the
 active crop rectangle to the default when the driver is first loaded,
 but not later.
diff --git a/Documentation/media/uapi/v4l/selection.svg 
b/Documentation/media/uapi/v4l/selection.svg
index a93e3b59786d..911062bd2844 100644
--- a/Documentation/media/uapi/v4l/selection.svg
+++ b/Documentation/media/uapi/v4l/selection.svg
@@ -1128,11 +1128,11 @@

   
   
-   COMPOSE_BONDS
+   COMPOSE_BOUNDS
   
   

-CROP_BONDS
+CROP_BOUNDS


 overscan area
-- 
2.7.4

[PATCH 4/5] media: docs: selection: improve formatting

[Luca Ceresoli]

Split section "Comparison with old cropping API" in paragraphs for
easier reading and improve visible links text.

Cc: Hans Verkuil <hverk...@xs4all.nl>
Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
---
 .../media/uapi/v4l/selection-api-vs-crop-api.rst   | 55 --
 1 file changed, 29 insertions(+), 26 deletions(-)

diff --git a/Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst 
b/Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst
index 2ad30a49184f..ba1064a244a0 100644
--- a/Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst
+++ b/Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst
@@ -6,31 +6,34 @@
 Comparison with old cropping API
 
 
-The selection API was introduced to cope with deficiencies of previous
-:ref:`API `, that was designed to control simple capture
-devices. Later the cropping API was adopted by video output drivers. The
-ioctls are used to select a part of the display were the video signal is
-inserted. It should be considered as an API abuse because the described
-operation is actually the composing. The selection API makes a clear
-distinction between composing and cropping operations by setting the
-appropriate targets. The V4L2 API lacks any support for composing to and
-cropping from an image inside a memory buffer. The application could
-configure a capture device to fill only a part of an image by abusing
-V4L2 API. Cropping a smaller image from a larger one is achieved by
-setting the field ``bytesperline`` at struct
-:c:type:`v4l2_pix_format`.
-Introducing an image offsets could be done by modifying field ``m_userptr``
-at struct
-:c:type:`v4l2_buffer` before calling
-:ref:`VIDIOC_QBUF`. Those operations should be avoided because they are not
-portable (endianness), and do not work for macroblock and Bayer formats
-and mmap buffers. The selection API deals with configuration of buffer
+The selection API was introduced to cope with deficiencies of the
+older :ref:`CROP API `, that was designed to control simple
+capture devices. Later the cropping API was adopted by video output
+drivers. The ioctls are used to select a part of the display were the
+video signal is inserted. It should be considered as an API abuse
+because the described operation is actually the composing. The
+selection API makes a clear distinction between composing and cropping
+operations by setting the appropriate targets.
+
+The V4L2 API lacks any support for composing to and cropping from an
+image inside a memory buffer. The application could configure a
+capture device to fill only a part of an image by abusing V4L2
+API. Cropping a smaller image from a larger one is achieved by setting
+the field ``bytesperline`` at struct :c:type:`v4l2_pix_format`.
+Introducing an image offsets could be done by modifying field
+``m_userptr`` at struct :c:type:`v4l2_buffer` before calling
+:ref:`VIDIOC_QBUF `. Those operations should be avoided
+because they are not portable (endianness), and do not work for
+macroblock and Bayer formats and mmap buffers.
+
+The selection API deals with configuration of buffer
 cropping/composing in a clear, intuitive and portable way. Next, with
 the selection API the concepts of the padded target and constraints
-flags are introduced. Finally, struct :c:type:`v4l2_crop`
-and struct :c:type:`v4l2_cropcap` have no reserved
-fields. Therefore there is no way to extend their functionality. The new
-struct :c:type:`v4l2_selection` provides a lot of place
-for future extensions. Driver developers are encouraged to implement
-only selection API. The former cropping API would be simulated using the
-new one.
+flags are introduced. Finally, struct :c:type:`v4l2_crop` and struct
+:c:type:`v4l2_cropcap` have no reserved fields. Therefore there is no
+way to extend their functionality. The new struct
+:c:type:`v4l2_selection` provides a lot of place for future
+extensions.
+
+Driver developers are encouraged to implement only selection API. The
+former cropping API would be simulated using the new one.
-- 
2.7.4

[PATCH 5/5] media: docs: selection: fix misleading sentence about the CROP API

[Luca Ceresoli]

The API limitation described here is about the CROP API, not about the
entire V4L2.

Cc: Hans Verkuil <hverk...@xs4all.nl>
Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
---
 Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst 
b/Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst
index ba1064a244a0..e7455fb1e572 100644
--- a/Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst
+++ b/Documentation/media/uapi/v4l/selection-api-vs-crop-api.rst
@@ -15,7 +15,7 @@ because the described operation is actually the composing. The
 selection API makes a clear distinction between composing and cropping
 operations by setting the appropriate targets.
 
-The V4L2 API lacks any support for composing to and cropping from an
+The CROP API lacks any support for composing to and cropping from an
 image inside a memory buffer. The application could configure a
 capture device to fill only a part of an image by abusing V4L2
 API. Cropping a smaller image from a larger one is achieved by setting
-- 
2.7.4

[PATCH] media: doc: fix ReST link syntax

[Luca Ceresoli]

There is a ':' i excess, resulting in an unwanted ':' in the rendered
output.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
---
 Documentation/media/kapi/v4l2-dev.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/media/kapi/v4l2-dev.rst 
b/Documentation/media/kapi/v4l2-dev.rst
index 7bb0505b60f1..eb03ccc41c41 100644
--- a/Documentation/media/kapi/v4l2-dev.rst
+++ b/Documentation/media/kapi/v4l2-dev.rst
@@ -31,7 +31,7 @@ of the video device exits.
 The default :c:func:`video_device_release` callback currently
 just calls ``kfree`` to free the allocated memory.
 
-There is also a ::c:func:`video_device_release_empty` function that does
+There is also a :c:func:`video_device_release_empty` function that does
 nothing (is empty) and should be used if the struct is embedded and there
 is nothing to do when it is released.
 
-- 
2.7.4

[PATCH v2 2/3] media: vb2-core: document the REQUEUEING state

[Luca Ceresoli]

VB2_BUF_STATE_REQUEUEING is accepted by vb2_buffer_done() but not
documented, so add it along with notes about calls in interrupt
context.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
Cc: Pawel Osciak <pa...@osciak.com>
Cc: Marek Szyprowski <m.szyprow...@samsung.com>
Cc: Kyungmin Park <kyungmin.p...@samsung.com>
Cc: Mauro Carvalho Chehab <mche...@kernel.org>

---
Changes v1 -> v2: none.
---
 include/media/videobuf2-core.h | 24 +++-
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index f1a479060f9e..f2887d3c 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -358,12 +358,12 @@ struct vb2_buffer {
  * driver can return an error if hardware fails, in that
  * case all buffers that have been already given by
  * the @buf_queue callback are to be returned by the driver
- * by calling vb2_buffer_done() with %VB2_BUF_STATE_QUEUED.
- * If you need a minimum number of buffers before you can
- * start streaming, then set
- * _queue->min_buffers_needed. If that is non-zero then
- * @start_streaming won't be called until at least that
- * many buffers have been queued up by userspace.
+ * by calling vb2_buffer_done() with %VB2_BUF_STATE_QUEUED
+ * or %VB2_BUF_STATE_REQUEUEING. If you need a minimum
+ * number of buffers before you can start streaming, then
+ * set _queue->min_buffers_needed. If that is non-zero
+ * then @start_streaming won't be called until at least
+ * that many buffers have been queued up by userspace.
  * @stop_streaming:called when 'streaming' state must be disabled; driver
  * should stop any DMA transactions or wait until they
  * finish and give back all buffers it got from _queue
@@ -601,8 +601,9 @@ void *vb2_plane_cookie(struct vb2_buffer *vb, unsigned int 
plane_no);
  * @state: state of the buffer, as defined by  vb2_buffer_state.
  * Either %VB2_BUF_STATE_DONE if the operation finished
  * successfully, %VB2_BUF_STATE_ERROR if the operation finished
- * with an error or %VB2_BUF_STATE_QUEUED if the driver wants to
- * requeue buffers.
+ * with an error or any of %VB2_BUF_STATE_QUEUED or
+ * %VB2_BUF_STATE_REQUEUEING if the driver wants to
+ * requeue buffers (see below).
  *
  * This function should be called by the driver after a hardware operation on
  * a buffer is finished and the buffer may be returned to userspace. The driver
@@ -613,7 +614,12 @@ void *vb2_plane_cookie(struct vb2_buffer *vb, unsigned int 
plane_no);
  * While streaming a buffer can only be returned in state DONE or ERROR.
  * The _ops->start_streaming op can also return them in case the DMA engine
  * cannot be started for some reason. In that case the buffers should be
- * returned with state QUEUED to put them back into the queue.
+ * returned with state QUEUED or REQUEUEING to put them back into the queue.
+ *
+ * %VB2_BUF_STATE_REQUEUEING is like %VB2_BUF_STATE_QUEUED, but it also calls
+ * _ops->buf_queue to queue buffers back to the driver. Note that calling
+ * vb2_buffer_done(..., VB2_BUF_STATE_REQUEUEING) from interrupt context will
+ * result in _ops->buf_queue being called in interrupt context as well.
  */
 void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state);
 
-- 
2.7.4

[PATCH v2 1/3] media: vb2-core: vb2_buffer_done: consolidate docs

[Luca Ceresoli]

Documentation about what start_streaming() should do on failure are
scattered in two places and mostly duplicated, so consolidate them in
one of the two places.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
Cc: Pawel Osciak <pa...@osciak.com>
Cc: Marek Szyprowski <m.szyprow...@samsung.com>
Cc: Kyungmin Park <kyungmin.p...@samsung.com>
Cc: Mauro Carvalho Chehab <mche...@kernel.org>

---
Changes v1 -> v2: none.
---
 include/media/videobuf2-core.h | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 5b6c541e4e1b..f1a479060f9e 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -602,9 +602,7 @@ void *vb2_plane_cookie(struct vb2_buffer *vb, unsigned int 
plane_no);
  * Either %VB2_BUF_STATE_DONE if the operation finished
  * successfully, %VB2_BUF_STATE_ERROR if the operation finished
  * with an error or %VB2_BUF_STATE_QUEUED if the driver wants to
- * requeue buffers. If start_streaming fails then it should return
- * buffers with state %VB2_BUF_STATE_QUEUED to put them back into
- * the queue.
+ * requeue buffers.
  *
  * This function should be called by the driver after a hardware operation on
  * a buffer is finished and the buffer may be returned to userspace. The driver
@@ -613,9 +611,9 @@ void *vb2_plane_cookie(struct vb2_buffer *vb, unsigned int 
plane_no);
  * to the driver by _ops->buf_queue can be passed to this function.
  *
  * While streaming a buffer can only be returned in state DONE or ERROR.
- * The start_streaming op can also return them in case the DMA engine cannot
- * be started for some reason. In that case the buffers should be returned with
- * state QUEUED.
+ * The _ops->start_streaming op can also return them in case the DMA engine
+ * cannot be started for some reason. In that case the buffers should be
+ * returned with state QUEUED to put them back into the queue.
  */
 void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state);
 
-- 
2.7.4

[PATCH v2 3/3] media: vb2-core: vb2_ops: document non-interrupt-context calling

[Luca Ceresoli]

Driver writers can benefit in knowing if/when callbacks are called in
interrupt context. But it is not completely obvious here, so document
it.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
Cc: Pawel Osciak <pa...@osciak.com>
Cc: Marek Szyprowski <m.szyprow...@samsung.com>
Cc: Kyungmin Park <kyungmin.p...@samsung.com>
Cc: Mauro Carvalho Chehab <mche...@kernel.org>

---
Changes v1 -> v2:
 - Fix typo in the title.
---
 include/media/videobuf2-core.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index f2887d3c..f6818f732f34 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -296,6 +296,9 @@ struct vb2_buffer {
 /**
  * struct vb2_ops - driver-specific callbacks.
  *
+ * These operations are not called from interrupt context except where
+ * mentioned specifically.
+ *
  * @queue_setup:   called from VIDIOC_REQBUFS() and VIDIOC_CREATE_BUFS()
  * handlers before memory allocation. It can be called
  * twice: if the original number of requested buffers
-- 
2.7.4

Re: [PATCH 3/3] media: vb2-core: vb2_ops: document non-interrupt-cantext calling

[Luca Ceresoli]

Hi,

I noticed a typo in the title:
cantext -> context

I will fix in v2.

On 28/02/2018 23:24, Luca Ceresoli wrote:
> Driver writers can benefit in knowing if/when callbacks are called in
> interrupt context. But it is not completely obvious here, so document it.
> 
> Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
> Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> Cc: Pawel Osciak <pa...@osciak.com>
> Cc: Marek Szyprowski <m.szyprow...@samsung.com>
> Cc: Kyungmin Park <kyungmin.p...@samsung.com>
> Cc: Mauro Carvalho Chehab <mche...@kernel.org>
> ---
>  include/media/videobuf2-core.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index f2887d3c..f6818f732f34 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -296,6 +296,9 @@ struct vb2_buffer {
>  /**
>   * struct vb2_ops - driver-specific callbacks.
>   *
> + * These operations are not called from interrupt context except where
> + * mentioned specifically.
> + *
>   * @queue_setup: called from VIDIOC_REQBUFS() and VIDIOC_CREATE_BUFS()
>   *   handlers before memory allocation. It can be called
>   *   twice: if the original number of requested buffers
> 


-- 
Luca

[PATCH] media: doc: poll: fix links to dual-ioctl sections

[Luca Ceresoli]

Links like :ref:`VIDIOC_STREAMON` expand to "ioctl VIDIOC_STREAMON,
VIDIOC_STREAMOFF". Thus our reader will think we are talking about
STREAMON _and_ STREAMOFF, but only one of the two actually applies
in some cases.

Fix by adding a link title, so the reader will read only the correct
ioctl name.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
Cc: Mauro Carvalho Chehab <mche...@kernel.org>
---
 Documentation/media/uapi/v4l/func-poll.rst | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/media/uapi/v4l/func-poll.rst 
b/Documentation/media/uapi/v4l/func-poll.rst
index d0432dc09b05..360bc6523ae2 100644
--- a/Documentation/media/uapi/v4l/func-poll.rst
+++ b/Documentation/media/uapi/v4l/func-poll.rst
@@ -39,7 +39,7 @@ When streaming I/O has been negotiated this function waits 
until a
 buffer has been filled by the capture device and can be dequeued with
 the :ref:`VIDIOC_DQBUF ` ioctl. For output devices this
 function waits until the device is ready to accept a new buffer to be
-queued up with the :ref:`VIDIOC_QBUF` ioctl for
+queued up with the :ref:`VIDIOC_QBUF ` ioctl for
 display. When buffers are already in the outgoing queue of the driver
 (capture) or the incoming queue isn't full (display) the function
 returns immediately.
@@ -52,11 +52,11 @@ flags in the ``revents`` field, output devices the 
``POLLOUT`` and
 ``POLLWRNORM`` flags. When the function timed out it returns a value of
 zero, on failure it returns -1 and the ``errno`` variable is set
 appropriately. When the application did not call
-:ref:`VIDIOC_STREAMON` the :ref:`poll() `
+:ref:`VIDIOC_STREAMON ` the :ref:`poll() `
 function succeeds, but sets the ``POLLERR`` flag in the ``revents``
 field. When the application has called
-:ref:`VIDIOC_STREAMON` for a capture device but
-hasn't yet called :ref:`VIDIOC_QBUF`, the
+:ref:`VIDIOC_STREAMON ` for a capture device but
+hasn't yet called :ref:`VIDIOC_QBUF `, the
 :ref:`poll() ` function succeeds and sets the ``POLLERR`` flag in
 the ``revents`` field. For output devices this same situation will cause
 :ref:`poll() ` to succeed as well, but it sets the ``POLLOUT`` and
-- 
2.7.4

[PATCH 3/3] media: vb2-core: vb2_ops: document non-interrupt-cantext calling

[Luca Ceresoli]

Driver writers can benefit in knowing if/when callbacks are called in
interrupt context. But it is not completely obvious here, so document it.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
Cc: Pawel Osciak <pa...@osciak.com>
Cc: Marek Szyprowski <m.szyprow...@samsung.com>
Cc: Kyungmin Park <kyungmin.p...@samsung.com>
Cc: Mauro Carvalho Chehab <mche...@kernel.org>
---
 include/media/videobuf2-core.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index f2887d3c..f6818f732f34 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -296,6 +296,9 @@ struct vb2_buffer {
 /**
  * struct vb2_ops - driver-specific callbacks.
  *
+ * These operations are not called from interrupt context except where
+ * mentioned specifically.
+ *
  * @queue_setup:   called from VIDIOC_REQBUFS() and VIDIOC_CREATE_BUFS()
  * handlers before memory allocation. It can be called
  * twice: if the original number of requested buffers
-- 
2.7.4

[PATCH 2/3] media: vb2-core: document the REQUEUEING state

[Luca Ceresoli]

VB2_BUF_STATE_REQUEUEING is accepted by vb2_buffer_done() but not
documented, so add it along with notes about calls in interrupt context.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
Cc: Pawel Osciak <pa...@osciak.com>
Cc: Marek Szyprowski <m.szyprow...@samsung.com>
Cc: Kyungmin Park <kyungmin.p...@samsung.com>
Cc: Mauro Carvalho Chehab <mche...@kernel.org>
---
 include/media/videobuf2-core.h | 24 +++-
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index f1a479060f9e..f2887d3c 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -358,12 +358,12 @@ struct vb2_buffer {
  * driver can return an error if hardware fails, in that
  * case all buffers that have been already given by
  * the @buf_queue callback are to be returned by the driver
- * by calling vb2_buffer_done() with %VB2_BUF_STATE_QUEUED.
- * If you need a minimum number of buffers before you can
- * start streaming, then set
- * _queue->min_buffers_needed. If that is non-zero then
- * @start_streaming won't be called until at least that
- * many buffers have been queued up by userspace.
+ * by calling vb2_buffer_done() with %VB2_BUF_STATE_QUEUED
+ * or %VB2_BUF_STATE_REQUEUEING. If you need a minimum
+ * number of buffers before you can start streaming, then
+ * set _queue->min_buffers_needed. If that is non-zero
+ * then @start_streaming won't be called until at least
+ * that many buffers have been queued up by userspace.
  * @stop_streaming:called when 'streaming' state must be disabled; driver
  * should stop any DMA transactions or wait until they
  * finish and give back all buffers it got from _queue
@@ -601,8 +601,9 @@ void *vb2_plane_cookie(struct vb2_buffer *vb, unsigned int 
plane_no);
  * @state: state of the buffer, as defined by  vb2_buffer_state.
  * Either %VB2_BUF_STATE_DONE if the operation finished
  * successfully, %VB2_BUF_STATE_ERROR if the operation finished
- * with an error or %VB2_BUF_STATE_QUEUED if the driver wants to
- * requeue buffers.
+ * with an error or any of %VB2_BUF_STATE_QUEUED or
+ * %VB2_BUF_STATE_REQUEUEING if the driver wants to
+ * requeue buffers (see below).
  *
  * This function should be called by the driver after a hardware operation on
  * a buffer is finished and the buffer may be returned to userspace. The driver
@@ -613,7 +614,12 @@ void *vb2_plane_cookie(struct vb2_buffer *vb, unsigned int 
plane_no);
  * While streaming a buffer can only be returned in state DONE or ERROR.
  * The _ops->start_streaming op can also return them in case the DMA engine
  * cannot be started for some reason. In that case the buffers should be
- * returned with state QUEUED to put them back into the queue.
+ * returned with state QUEUED or REQUEUEING to put them back into the queue.
+ *
+ * %VB2_BUF_STATE_REQUEUEING is like %VB2_BUF_STATE_QUEUED, but it also calls
+ * _ops->buf_queue to queue buffers back to the driver. Note that calling
+ * vb2_buffer_done(..., VB2_BUF_STATE_REQUEUEING) from interrupt context will
+ * result in _ops->buf_queue being called in interrupt context as well.
  */
 void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state);
 
-- 
2.7.4

[PATCH 1/3] media: vb2-core: vb2_buffer_done: consolidate docs

[Luca Ceresoli]

Documentation about what start_streaming() should do on failure are
scattered in two places and mostly duplicated, so consolidate them in
one of the two places.

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
Cc: Pawel Osciak <pa...@osciak.com>
Cc: Marek Szyprowski <m.szyprow...@samsung.com>
Cc: Kyungmin Park <kyungmin.p...@samsung.com>
Cc: Mauro Carvalho Chehab <mche...@kernel.org>
---
 include/media/videobuf2-core.h | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 5b6c541e4e1b..f1a479060f9e 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -602,9 +602,7 @@ void *vb2_plane_cookie(struct vb2_buffer *vb, unsigned int 
plane_no);
  * Either %VB2_BUF_STATE_DONE if the operation finished
  * successfully, %VB2_BUF_STATE_ERROR if the operation finished
  * with an error or %VB2_BUF_STATE_QUEUED if the driver wants to
- * requeue buffers. If start_streaming fails then it should return
- * buffers with state %VB2_BUF_STATE_QUEUED to put them back into
- * the queue.
+ * requeue buffers.
  *
  * This function should be called by the driver after a hardware operation on
  * a buffer is finished and the buffer may be returned to userspace. The driver
@@ -613,9 +611,9 @@ void *vb2_plane_cookie(struct vb2_buffer *vb, unsigned int 
plane_no);
  * to the driver by _ops->buf_queue can be passed to this function.
  *
  * While streaming a buffer can only be returned in state DONE or ERROR.
- * The start_streaming op can also return them in case the DMA engine cannot
- * be started for some reason. In that case the buffers should be returned with
- * state QUEUED.
+ * The _ops->start_streaming op can also return them in case the DMA engine
+ * cannot be started for some reason. In that case the buffers should be
+ * returned with state QUEUED to put them back into the queue.
  */
 void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state);
 
-- 
2.7.4

[PATCH] media: imx274: fix typo in error message

[Luca Ceresoli]

Signed-off-by: Luca Ceresoli <l...@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 664e8acdf2a0..daec33f4196a 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1426,7 +1426,7 @@ static int imx274_set_vflip(struct stimx274 *priv, int 
val)
 
err = imx274_write_reg(priv, IMX274_VFLIP_REG, val);
if (err) {
-   dev_err(>client->dev, "VFILP control error\n");
+   dev_err(>client->dev, "VFLIP control error\n");
return err;
}
 
-- 
2.7.4