[REVIEW PATCH 0/7] smiapp: Cleanups, small bugfixes

[REVIEW PATCH 0/7] smiapp: Cleanups, small bugfixes

[Sakari Ailus]

Hi,

These patches contain cleanups, primarily in the sensor initialisation, and
replace the pll_flags quirk by a generic init quirk which, besides setting
pll flags, can be used to e.g. creating sensor specific controls.

-- 
Kind regards,
Sakari

[REVIEW PATCH 1/7] smiapp: Access flash capabilities through limits

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

The flash capability register is already read as part of the limit
registers. Do no access it separately; instead use the value from the
limits.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c |    9 +--------
 drivers/media/i2c/smiapp/smiapp.h      |    1 -
 2 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 48e1a1f..4f36be5 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -1483,7 +1483,7 @@ static int smiapp_start_streaming(struct smiapp_sensor *sensor)
 	if (rval < 0)
 		goto out;
 
-	if ((sensor->flash_capability &
+	if ((sensor->limits[SMIAPP_LIMIT_FLASH_MODE_CAPABILITY] &
 	     (SMIAPP_FLASH_MODE_CAPABILITY_SINGLE_STROBE |
 	      SMIAPP_FLASH_MODE_CAPABILITY_MULTIPLE_STROBE)) &&
 	    sensor->platform_data->strobe_setup != NULL &&
@@ -2529,7 +2529,6 @@ static int smiapp_init(struct smiapp_sensor *sensor)
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
 	struct smiapp_pll *pll = &sensor->pll;
 	struct smiapp_subdev *last = NULL;
-	u32 tmp;
 	unsigned int i;
 	int rval;
 
@@ -2785,12 +2784,6 @@ static int smiapp_init(struct smiapp_sensor *sensor)
 	sensor->streaming = false;
 	sensor->dev_init_done = true;
 
-	/* check flash capability */
-	rval = smiapp_read(sensor, SMIAPP_REG_U8_FLASH_MODE_CAPABILITY, &tmp);
-	sensor->flash_capability = tmp;
-	if (rval)
-		goto out_cleanup;
-
 	smiapp_power_off(sensor);
 
 	return 0;
diff --git a/drivers/media/i2c/smiapp/smiapp.h b/drivers/media/i2c/smiapp/smiapp.h
index 8fded46..ed010a8 100644
--- a/drivers/media/i2c/smiapp/smiapp.h
+++ b/drivers/media/i2c/smiapp/smiapp.h
@@ -216,7 +216,6 @@ struct smiapp_sensor {
 	u8 scaling_mode;
 
 	u8 hvflip_inv_mask; /* H/VFLIP inversion due to sensor orientation */
-	u8 flash_capability;
 	u8 frame_skip;
 
 	int power_count;
-- 
1.7.10.4

[REVIEW PATCH 2/7] smiapp: Free control handlers in sub-device cleanup

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

Also call smiapp_cleanup() in smiapp_remove(), replacing code that did the
same than the function.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 4f36be5..72a0de6 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2522,6 +2522,8 @@ static void smiapp_cleanup(struct smiapp_sensor *sensor)
 
 	device_remove_file(&client->dev, &dev_attr_nvm);
 	device_remove_file(&client->dev, &dev_attr_ident);
+
+	smiapp_free_controls(sensor);
 }
 
 static int smiapp_init(struct smiapp_sensor *sensor)
@@ -3124,15 +3126,11 @@ static int smiapp_remove(struct i2c_client *client)
 		sensor->power_count = 0;
 	}
 
-	device_remove_file(&client->dev, &dev_attr_ident);
-	if (sensor->nvm)
-		device_remove_file(&client->dev, &dev_attr_nvm);
-
 	for (i = 0; i < sensor->ssds_used; i++) {
 		v4l2_device_unregister_subdev(&sensor->ssds[i].sd);
 		media_entity_cleanup(&sensor->ssds[i].sd.entity);
 	}
-	smiapp_free_controls(sensor);
+	smiapp_cleanup(sensor);
 
 	return 0;
 }
-- 
1.7.10.4

[REVIEW PATCH 3/7] smiapp: Clean up smiapp_init_controls()

[Sakari Ailus]

Clean up smiapp_init_controls() by adding newlines to appropriate places and
by removing superfluous error handling. The caller will clean up control
handlers in any case if the function fails.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c |   19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 72a0de6..66d94c3 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -527,6 +527,7 @@ static int smiapp_init_controls(struct smiapp_sensor *sensor)
 	rval = v4l2_ctrl_handler_init(&sensor->pixel_array->ctrl_handler, 12);
 	if (rval)
 		return rval;
+
 	sensor->pixel_array->ctrl_handler.lock = &sensor->mutex;
 
 	sensor->analog_gain = v4l2_ctrl_new_std(
@@ -585,8 +586,7 @@ static int smiapp_init_controls(struct smiapp_sensor *sensor)
 		dev_err(&client->dev,
 			"pixel array controls initialization failed (%d)\n",
 			sensor->pixel_array->ctrl_handler.error);
-		rval = sensor->pixel_array->ctrl_handler.error;
-		goto error;
+		return sensor->pixel_array->ctrl_handler.error;
 	}
 
 	sensor->pixel_array->sd.ctrl_handler =
@@ -596,7 +596,8 @@ static int smiapp_init_controls(struct smiapp_sensor *sensor)
 
 	rval = v4l2_ctrl_handler_init(&sensor->src->ctrl_handler, 0);
 	if (rval)
-		goto error;
+		return rval;
+
 	sensor->src->ctrl_handler.lock = &sensor->mutex;
 
 	for (max = 0; sensor->platform_data->op_sys_clock[max + 1]; max++);
@@ -614,20 +615,12 @@ static int smiapp_init_controls(struct smiapp_sensor *sensor)
 		dev_err(&client->dev,
 			"src controls initialization failed (%d)\n",
 			sensor->src->ctrl_handler.error);
-		rval = sensor->src->ctrl_handler.error;
-		goto error;
+		return sensor->src->ctrl_handler.error;
 	}
 
-	sensor->src->sd.ctrl_handler =
-		&sensor->src->ctrl_handler;
+	sensor->src->sd.ctrl_handler = &sensor->src->ctrl_handler;
 
 	return 0;
-
-error:
-	v4l2_ctrl_handler_free(&sensor->pixel_array->ctrl_handler);
-	v4l2_ctrl_handler_free(&sensor->src->ctrl_handler);
-
-	return rval;
 }
 
 static void smiapp_free_controls(struct smiapp_sensor *sensor)
-- 
1.7.10.4

[REVIEW PATCH 4/7] smiapp: Separate late controls from the rest

[Sakari Ailus]

The default values and limits for certain controls need the knowledge of
available media bus codes or link frequencies. Create such controls later
on, so that most of the initialisation of the sensor has already been done
when the init quirk is called.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c |   54 +++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 19 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 66d94c3..e8e88bd 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -519,9 +519,6 @@ static const struct v4l2_ctrl_ops smiapp_ctrl_ops = {
 static int smiapp_init_controls(struct smiapp_sensor *sensor)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
-	unsigned long *valid_link_freqs = &sensor->valid_link_freqs[
-		sensor->csi_format->compressed - SMIAPP_COMPRESSED_BASE];
-	unsigned int max, i;
 	int rval;
 
 	rval = v4l2_ctrl_handler_init(&sensor->pixel_array->ctrl_handler, 12);
@@ -573,15 +570,6 @@ static int smiapp_init_controls(struct smiapp_sensor *sensor)
 				     ARRAY_SIZE(smiapp_test_patterns) - 1,
 				     0, 0, smiapp_test_patterns);
 
-	for (i = 0; i < ARRAY_SIZE(sensor->test_data); i++) {
-		int max_value = (1 << sensor->csi_format->width) - 1;
-		sensor->test_data[i] =
-			v4l2_ctrl_new_std(
-				&sensor->pixel_array->ctrl_handler,
-				&smiapp_ctrl_ops, V4L2_CID_TEST_PATTERN_RED + i,
-				0, max_value, 1, max_value);
-	}
-
 	if (sensor->pixel_array->ctrl_handler.error) {
 		dev_err(&client->dev,
 			"pixel array controls initialization failed (%d)\n",
@@ -600,13 +588,6 @@ static int smiapp_init_controls(struct smiapp_sensor *sensor)
 
 	sensor->src->ctrl_handler.lock = &sensor->mutex;
 
-	for (max = 0; sensor->platform_data->op_sys_clock[max + 1]; max++);
-
-	sensor->link_freq = v4l2_ctrl_new_int_menu(
-		&sensor->src->ctrl_handler, &smiapp_ctrl_ops,
-		V4L2_CID_LINK_FREQ, __fls(*valid_link_freqs),
-		__ffs(*valid_link_freqs), sensor->platform_data->op_sys_clock);
-
 	sensor->pixel_rate_csi = v4l2_ctrl_new_std(
 		&sensor->src->ctrl_handler, &smiapp_ctrl_ops,
 		V4L2_CID_PIXEL_RATE, 1, INT_MAX, 1, 1);
@@ -623,6 +604,35 @@ static int smiapp_init_controls(struct smiapp_sensor *sensor)
 	return 0;
 }
 
+/*
+ * For controls that require information on available media bus codes
+ * and linke frequencies.
+ */
+static int smiapp_init_late_controls(struct smiapp_sensor *sensor)
+{
+	unsigned long *valid_link_freqs = &sensor->valid_link_freqs[
+		sensor->csi_format->compressed - SMIAPP_COMPRESSED_BASE];
+	unsigned int max, i;
+
+	for (i = 0; i < ARRAY_SIZE(sensor->test_data); i++) {
+		int max_value = (1 << sensor->csi_format->width) - 1;
+		sensor->test_data[i] =
+			v4l2_ctrl_new_std(
+				&sensor->pixel_array->ctrl_handler,
+				&smiapp_ctrl_ops, V4L2_CID_TEST_PATTERN_RED + i,
+				0, max_value, 1, max_value);
+	}
+
+	for (max = 0; sensor->platform_data->op_sys_clock[max + 1]; max++);
+
+	sensor->link_freq = v4l2_ctrl_new_int_menu(
+		&sensor->src->ctrl_handler, &smiapp_ctrl_ops,
+		V4L2_CID_LINK_FREQ, __fls(*valid_link_freqs),
+		__ffs(*valid_link_freqs), sensor->platform_data->op_sys_clock);
+
+	return sensor->src->ctrl_handler.error;
+}
+
 static void smiapp_free_controls(struct smiapp_sensor *sensor)
 {
 	unsigned int i;
@@ -2768,6 +2778,12 @@ static int smiapp_init(struct smiapp_sensor *sensor)
 	if (rval < 0)
 		goto out_cleanup;
 
+	rval = smiapp_init_late_controls(sensor);
+	if (rval) {
+		rval = -ENODEV;
+		goto out_cleanup;
+	}
+
 	mutex_lock(&sensor->mutex);
 	rval = smiapp_update_mode(sensor);
 	mutex_unlock(&sensor->mutex);
-- 
1.7.10.4

[REVIEW PATCH 5/7] smiapp: Move enumerating available media bus codes later

[Sakari Ailus]

As the controls creation is separated in two sections, the available media
bus codes and link frequencies can be enumerated later on.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index e8e88bd..bacef3e 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2703,12 +2703,6 @@ static int smiapp_init(struct smiapp_sensor *sensor)
 	if (sensor->minfo.smiapp_profile == SMIAPP_PROFILE_0)
 		pll->flags |= SMIAPP_PLL_FLAG_NO_OP_CLOCKS;
 
-	rval = smiapp_get_mbus_formats(sensor);
-	if (rval) {
-		rval = -ENODEV;
-		goto out_cleanup;
-	}
-
 	for (i = 0; i < SMIAPP_SUBDEVS; i++) {
 		struct {
 			struct smiapp_subdev *ssd;
@@ -2778,6 +2772,12 @@ static int smiapp_init(struct smiapp_sensor *sensor)
 	if (rval < 0)
 		goto out_cleanup;
 
+	rval = smiapp_get_mbus_formats(sensor);
+	if (rval) {
+		rval = -ENODEV;
+		goto out_cleanup;
+	}
+
 	rval = smiapp_init_late_controls(sensor);
 	if (rval) {
 		rval = -ENODEV;
-- 
1.7.10.4

[REVIEW PATCH 6/7] smiapp: Replace pll_flags quirk with more generic init quirk

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

The pll_flags quirk just returned the extra PLL flags the sensor required,
but the init quirk is far more versatile. It can be used to perform any
extra initialisation needed by the sensor, including allocating memory for
sensor specific struct and creating sensor specific new controls.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c  |    5 ++++-
 drivers/media/i2c/smiapp/smiapp-quirk.c |    8 +++++---
 drivers/media/i2c/smiapp/smiapp-quirk.h |    4 ++++
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index bacef3e..737da1e 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2697,7 +2697,6 @@ static int smiapp_init(struct smiapp_sensor *sensor)
 	pll->bus_type = SMIAPP_PLL_BUS_TYPE_CSI2;
 	pll->csi2.lanes = sensor->platform_data->lanes;
 	pll->ext_clk_freq_hz = sensor->platform_data->ext_clk;
-	pll->flags = smiapp_call_quirk(sensor, pll_flags);
 	pll->scale_n = sensor->limits[SMIAPP_LIMIT_SCALER_N_MIN];
 	/* Profile 0 sensors have no separate OP clock branch. */
 	if (sensor->minfo.smiapp_profile == SMIAPP_PROFILE_0)
@@ -2772,6 +2771,10 @@ static int smiapp_init(struct smiapp_sensor *sensor)
 	if (rval < 0)
 		goto out_cleanup;
 
+	rval = smiapp_call_quirk(sensor, init);
+	if (rval)
+		goto out_cleanup;
+
 	rval = smiapp_get_mbus_formats(sensor);
 	if (rval) {
 		rval = -ENODEV;
diff --git a/drivers/media/i2c/smiapp/smiapp-quirk.c b/drivers/media/i2c/smiapp/smiapp-quirk.c
index dd4ae6f..abf9ea7 100644
--- a/drivers/media/i2c/smiapp/smiapp-quirk.c
+++ b/drivers/media/i2c/smiapp/smiapp-quirk.c
@@ -214,9 +214,11 @@ static int jt8ev1_post_streamoff(struct smiapp_sensor *sensor)
 	return smiapp_write_8(sensor, 0x3328, 0x80);
 }
 
-static unsigned long jt8ev1_pll_flags(struct smiapp_sensor *sensor)
+static int jt8ev1_init(struct smiapp_sensor *sensor)
 {
-	return SMIAPP_PLL_FLAG_OP_PIX_CLOCK_PER_LANE;
+	sensor->pll.flags |= SMIAPP_PLL_FLAG_OP_PIX_CLOCK_PER_LANE;
+
+	return 0;
 }
 
 const struct smiapp_quirk smiapp_jt8ev1_quirk = {
@@ -224,7 +226,7 @@ const struct smiapp_quirk smiapp_jt8ev1_quirk = {
 	.post_poweron = jt8ev1_post_poweron,
 	.pre_streamon = jt8ev1_pre_streamon,
 	.post_streamoff = jt8ev1_post_streamoff,
-	.pll_flags = jt8ev1_pll_flags,
+	.init = jt8ev1_init,
 };
 
 static int tcm8500md_limits(struct smiapp_sensor *sensor)
diff --git a/drivers/media/i2c/smiapp/smiapp-quirk.h b/drivers/media/i2c/smiapp/smiapp-quirk.h
index 3a3c3e5..a24eb43 100644
--- a/drivers/media/i2c/smiapp/smiapp-quirk.h
+++ b/drivers/media/i2c/smiapp/smiapp-quirk.h
@@ -29,6 +29,9 @@ struct smiapp_sensor;
  * @post_poweron: Called always after the sensor has been fully powered on.
  * @pre_streamon: Called just before streaming is enabled.
  * @post_streamon: Called right after stopping streaming.
+ * @pll_flags: Return flags for the PLL calculator.
+ * @init: Quirk initialisation, called the last in probe(). This is
+ *	  also appropriate for adding sensor specific controls, for instance.
  * @reg_access: Register access quirk. The quirk may divert the access
  *		to another register, or no register at all.
  *
@@ -47,6 +50,7 @@ struct smiapp_quirk {
 	int (*pre_streamon)(struct smiapp_sensor *sensor);
 	int (*post_streamoff)(struct smiapp_sensor *sensor);
 	unsigned long (*pll_flags)(struct smiapp_sensor *sensor);
+	int (*init)(struct smiapp_sensor *sensor);
 	int (*reg_access)(struct smiapp_sensor *sensor, bool write, u32 *reg,
 			  u32 *val);
 	unsigned long flags;
-- 
1.7.10.4

[REVIEW PATCH 7/7] smiapp: Add parentheses to macro arguments used in macros

[Sakari Ailus]

This makes the macros a little bit safer.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-quirk.h |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-quirk.h b/drivers/media/i2c/smiapp/smiapp-quirk.h
index a24eb43..dac5566 100644
--- a/drivers/media/i2c/smiapp/smiapp-quirk.h
+++ b/drivers/media/i2c/smiapp/smiapp-quirk.h
@@ -72,14 +72,14 @@ void smiapp_replace_limit(struct smiapp_sensor *sensor,
 		.val = _val,		\
 	}
 
-#define smiapp_call_quirk(_sensor, _quirk, ...)				\
-	(_sensor->minfo.quirk &&					\
-	 _sensor->minfo.quirk->_quirk ?					\
-	 _sensor->minfo.quirk->_quirk(_sensor, ##__VA_ARGS__) : 0)
+#define smiapp_call_quirk(sensor, _quirk, ...)				\
+	((sensor)->minfo.quirk &&					\
+	 (sensor)->minfo.quirk->_quirk ?				\
+	 (sensor)->minfo.quirk->_quirk(sensor, ##__VA_ARGS__) : 0)
 
-#define smiapp_needs_quirk(_sensor, _quirk)		\
-	(_sensor->minfo.quirk ?				\
-	 _sensor->minfo.quirk->flags & _quirk : 0)
+#define smiapp_needs_quirk(sensor, _quirk)		\
+	((sensor)->minfo.quirk ?			\
+	 (sensor)->minfo.quirk->flags & _quirk : 0)
 
 extern const struct smiapp_quirk smiapp_jt8ev1_quirk;
 extern const struct smiapp_quirk smiapp_imx125es_quirk;
-- 
1.7.10.4