[PATCH v2 00/17] More smiapp cleanups, fixes

[PATCH v2 00/17] More smiapp cleanups, fixes

[Sakari Ailus]

Hi all,

This set further cleans up the smiapp driver and prepares for later
changes.

More fixes and cleanups since v1.

-- 
Kind regards,
Sakari

[PATCH v2 01/17] smiapp: Move sub-device initialisation into a separate function

[Sakari Ailus]

Simplify smiapp_init() by moving the initialisation of individual
sub-devices to a separate function.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 44f8c7e..862017e 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2535,11 +2535,55 @@ static void smiapp_cleanup(struct smiapp_sensor *sensor)
 	smiapp_free_controls(sensor);
 }
 
+static void smiapp_create_subdev(struct smiapp_sensor *sensor,
+				 struct smiapp_subdev *ssd, const char *name)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
+
+	if (ssd != sensor->src)
+		v4l2_subdev_init(&ssd->sd, &smiapp_ops);
+
+	ssd->sensor = sensor;
+
+	if (ssd == sensor->pixel_array) {
+		ssd->npads = 1;
+	} else {
+		ssd->npads = 2;
+		ssd->source_pad = 1;
+	}
+
+	snprintf(ssd->sd.name,
+		 sizeof(ssd->sd.name), "%s %s %d-%4.4x", sensor->minfo.name,
+		 name, i2c_adapter_id(client->adapter), client->addr);
+
+	ssd->sink_fmt.width =
+		sensor->limits[SMIAPP_LIMIT_X_ADDR_MAX] + 1;
+	ssd->sink_fmt.height =
+		sensor->limits[SMIAPP_LIMIT_Y_ADDR_MAX] + 1;
+	ssd->compose.width = ssd->sink_fmt.width;
+	ssd->compose.height = ssd->sink_fmt.height;
+	ssd->crop[ssd->source_pad] = ssd->compose;
+	ssd->pads[ssd->source_pad].flags = MEDIA_PAD_FL_SOURCE;
+	if (ssd != sensor->pixel_array) {
+		ssd->crop[ssd->sink_pad] = ssd->compose;
+		ssd->pads[ssd->sink_pad].flags = MEDIA_PAD_FL_SINK;
+	}
+
+	ssd->sd.entity.ops = &smiapp_entity_ops;
+
+	if (ssd == sensor->src)
+		return;
+
+	ssd->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	ssd->sd.internal_ops = &smiapp_internal_ops;
+	ssd->sd.owner = THIS_MODULE;
+	v4l2_set_subdevdata(&ssd->sd, client);
+}
+
 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;
 	unsigned int i;
 	int rval;
 
@@ -2700,64 +2744,10 @@ static int smiapp_init(struct smiapp_sensor *sensor)
 	if (sensor->minfo.smiapp_profile == SMIAPP_PROFILE_0)
 		pll->flags |= SMIAPP_PLL_FLAG_NO_OP_CLOCKS;
 
-	for (i = 0; i < SMIAPP_SUBDEVS; i++) {
-		struct {
-			struct smiapp_subdev *ssd;
-			char *name;
-		} const __this[] = {
-			{ sensor->scaler, "scaler", },
-			{ sensor->binner, "binner", },
-			{ sensor->pixel_array, "pixel array", },
-		}, *_this = &__this[i];
-		struct smiapp_subdev *this = _this->ssd;
-
-		if (!this)
-			continue;
-
-		if (this != sensor->src)
-			v4l2_subdev_init(&this->sd, &smiapp_ops);
-
-		this->sensor = sensor;
-
-		if (this == sensor->pixel_array) {
-			this->npads = 1;
-		} else {
-			this->npads = 2;
-			this->source_pad = 1;
-		}
-
-		snprintf(this->sd.name,
-			 sizeof(this->sd.name), "%s %s %d-%4.4x",
-			 sensor->minfo.name, _this->name,
-			 i2c_adapter_id(client->adapter), client->addr);
-
-		this->sink_fmt.width =
-			sensor->limits[SMIAPP_LIMIT_X_ADDR_MAX] + 1;
-		this->sink_fmt.height =
-			sensor->limits[SMIAPP_LIMIT_Y_ADDR_MAX] + 1;
-		this->compose.width = this->sink_fmt.width;
-		this->compose.height = this->sink_fmt.height;
-		this->crop[this->source_pad] = this->compose;
-		this->pads[this->source_pad].flags = MEDIA_PAD_FL_SOURCE;
-		if (this != sensor->pixel_array) {
-			this->crop[this->sink_pad] = this->compose;
-			this->pads[this->sink_pad].flags = MEDIA_PAD_FL_SINK;
-		}
-
-		this->sd.entity.ops = &smiapp_entity_ops;
-
-		if (last == NULL) {
-			last = this;
-			continue;
-		}
-
-		this->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
-		this->sd.internal_ops = &smiapp_internal_ops;
-		this->sd.owner = THIS_MODULE;
-		v4l2_set_subdevdata(&this->sd, client);
-
-		last = this;
-	}
+	if (sensor->scaler)
+		smiapp_create_subdev(sensor, sensor->scaler, "scaler");
+	smiapp_create_subdev(sensor, sensor->binner, "binner");
+	smiapp_create_subdev(sensor, sensor->pixel_array, "pixel_array");
 
 	dev_dbg(&client->dev, "profile %d\n", sensor->minfo.smiapp_profile);
 
-- 
2.1.4

Re: [PATCH v2 01/17] smiapp: Move sub-device initialisation into a separate function

[Sebastian Reichel]

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

Hi,

On Thu, Sep 15, 2016 at 02:22:15PM +0300, Sakari Ailus wrote:
> Simplify smiapp_init() by moving the initialisation of individual
> sub-devices to a separate function.

Reviewed-By: Sebastian Reichel <sre@kernel.org>

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/i2c/smiapp/smiapp-core.c | 108 +++++++++++++++------------------
>  1 file changed, 49 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
> index 44f8c7e..862017e 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
>
> [...]
>
> +	if (sensor->scaler)
> +		smiapp_create_subdev(sensor, sensor->scaler, "scaler");

I would add the NULL check to smiapp_create_subdev.

> +	smiapp_create_subdev(sensor, sensor->binner, "binner");
> +	smiapp_create_subdev(sensor, sensor->pixel_array, "pixel_array");
>  
>  	dev_dbg(&client->dev, "profile %d\n", sensor->minfo.smiapp_profile);
> 

-- Sebastian

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

Re: [PATCH v2 01/17] smiapp: Move sub-device initialisation into a separate function

[Sakari Ailus]

Hi Sebastian,

Thank you for the review!

Sebastian Reichel wrote:
> Hi,
>
> On Thu, Sep 15, 2016 at 02:22:15PM +0300, Sakari Ailus wrote:
>> Simplify smiapp_init() by moving the initialisation of individual
>> sub-devices to a separate function.
>
> Reviewed-By: Sebastian Reichel <sre@kernel.org>
>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>>   drivers/media/i2c/smiapp/smiapp-core.c | 108 +++++++++++++++------------------
>>   1 file changed, 49 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
>> index 44f8c7e..862017e 100644
>> --- a/drivers/media/i2c/smiapp/smiapp-core.c
>> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
>>
>> [...]
>>
>> +	if (sensor->scaler)
>> +		smiapp_create_subdev(sensor, sensor->scaler, "scaler");
>
> I would add the NULL check to smiapp_create_subdev.

I first thought I'd say it makes it cleaner what's optional and what's 
not. The same is however visible some ten--twenty lines above this code, 
so not really an argument for that. Will fix.

>
>> +	smiapp_create_subdev(sensor, sensor->binner, "binner");
>> +	smiapp_create_subdev(sensor, sensor->pixel_array, "pixel_array");
>>
>>   	dev_dbg(&client->dev, "profile %d\n", sensor->minfo.smiapp_profile);
>>
>
> -- Sebastian
>


-- 
Sakari Ailus
sakari.ailus@linux.intel.com

[PATCH v2 02/17] smiapp: Explicitly define number of pads in initialisation

[Sakari Ailus]

Define the number of pads explicitly in initialising the sub-devices.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 862017e..be74ba3 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2536,7 +2536,8 @@ static void smiapp_cleanup(struct smiapp_sensor *sensor)
 }
 
 static void smiapp_create_subdev(struct smiapp_sensor *sensor,
-				 struct smiapp_subdev *ssd, const char *name)
+				 struct smiapp_subdev *ssd, const char *name,
+				 unsigned short num_pads)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
 
@@ -2545,12 +2546,8 @@ static void smiapp_create_subdev(struct smiapp_sensor *sensor,
 
 	ssd->sensor = sensor;
 
-	if (ssd == sensor->pixel_array) {
-		ssd->npads = 1;
-	} else {
-		ssd->npads = 2;
-		ssd->source_pad = 1;
-	}
+	ssd->npads = num_pads;
+	ssd->source_pad = num_pads - 1;
 
 	snprintf(ssd->sd.name,
 		 sizeof(ssd->sd.name), "%s %s %d-%4.4x", sensor->minfo.name,
@@ -2745,9 +2742,9 @@ static int smiapp_init(struct smiapp_sensor *sensor)
 		pll->flags |= SMIAPP_PLL_FLAG_NO_OP_CLOCKS;
 
 	if (sensor->scaler)
-		smiapp_create_subdev(sensor, sensor->scaler, "scaler");
-	smiapp_create_subdev(sensor, sensor->binner, "binner");
-	smiapp_create_subdev(sensor, sensor->pixel_array, "pixel_array");
+		smiapp_create_subdev(sensor, sensor->scaler, "scaler", 2);
+	smiapp_create_subdev(sensor, sensor->binner, "binner", 2);
+	smiapp_create_subdev(sensor, sensor->pixel_array, "pixel_array", 1);
 
 	dev_dbg(&client->dev, "profile %d\n", sensor->minfo.smiapp_profile);
 
-- 
2.1.4

Re: [PATCH v2 02/17] smiapp: Explicitly define number of pads in initialisation

[Sebastian Reichel]

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

Hi,

On Thu, Sep 15, 2016 at 02:22:16PM +0300, Sakari Ailus wrote:
> Define the number of pads explicitly in initialising the sub-devices.

Reviewed-By: Sebastian Reichel <sre@kernel.org>

-- Sebastian

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

[PATCH v2 03/17] smiapp: Initialise media entity after sensor init

[Sakari Ailus]

This allows determining the number of pads in the entity based on the
sensor.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index be74ba3..0a03f30 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -3056,12 +3056,7 @@ static int smiapp_probe(struct i2c_client *client,
 	sensor->src->sd.internal_ops = &smiapp_internal_src_ops;
 	sensor->src->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 	sensor->src->sensor = sensor;
-
 	sensor->src->pads[0].flags = MEDIA_PAD_FL_SOURCE;
-	rval = media_entity_pads_init(&sensor->src->sd.entity, 2,
-				 sensor->src->pads);
-	if (rval < 0)
-		return rval;
 
 	if (client->dev.of_node) {
 		rval = smiapp_init(sensor);
@@ -3069,6 +3064,11 @@ static int smiapp_probe(struct i2c_client *client,
 			goto out_media_entity_cleanup;
 	}
 
+	rval = media_entity_pads_init(&sensor->src->sd.entity, 2,
+				 sensor->src->pads);
+	if (rval < 0)
+		goto out_media_entity_cleanup;
+
 	rval = v4l2_async_register_subdev(&sensor->src->sd);
 	if (rval < 0)
 		goto out_media_entity_cleanup;
-- 
2.1.4

Re: [PATCH v2 03/17] smiapp: Initialise media entity after sensor init

[Sebastian Reichel]

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

Hi,

On Thu, Sep 15, 2016 at 02:22:17PM +0300, Sakari Ailus wrote:
> This allows determining the number of pads in the entity based on the
> sensor.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/i2c/smiapp/smiapp-core.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
> index be74ba3..0a03f30 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -3056,12 +3056,7 @@ static int smiapp_probe(struct i2c_client *client,
>  	sensor->src->sd.internal_ops = &smiapp_internal_src_ops;
>  	sensor->src->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>  	sensor->src->sensor = sensor;
> -
>  	sensor->src->pads[0].flags = MEDIA_PAD_FL_SOURCE;
> -	rval = media_entity_pads_init(&sensor->src->sd.entity, 2,
> -				 sensor->src->pads);
> -	if (rval < 0)
> -		return rval;
>  
>  	if (client->dev.of_node) {
>  		rval = smiapp_init(sensor);
> @@ -3069,6 +3064,11 @@ static int smiapp_probe(struct i2c_client *client,
>  			goto out_media_entity_cleanup;
>  	}
>  
> +	rval = media_entity_pads_init(&sensor->src->sd.entity, 2,
> +				 sensor->src->pads);
> +	if (rval < 0)
> +		goto out_media_entity_cleanup;
> +
>  	rval = v4l2_async_register_subdev(&sensor->src->sd);
>  	if (rval < 0)
>  		goto out_media_entity_cleanup;

As far as I can see this is not strictly needed, but:

Reviewed-By: Sebastian Reichel <sre@kernel.org>

-- Sebastian

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

[PATCH v2 04/17] smiapp: Split off sub-device registration into two

[Sakari Ailus]

Remove the loop in sub-device registration and create each sub-device
explicitly instead.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 0a03f30..9022ffc 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2475,54 +2475,62 @@ static const struct v4l2_subdev_ops smiapp_ops;
 static const struct v4l2_subdev_internal_ops smiapp_internal_ops;
 static const struct media_entity_operations smiapp_entity_ops;
 
-static int smiapp_register_subdevs(struct smiapp_sensor *sensor)
+static int smiapp_register_subdev(struct smiapp_sensor *sensor,
+				  struct smiapp_subdev *ssd,
+				  struct smiapp_subdev *sink_ssd,
+				  u16 source_pad, u16 sink_pad, u32 link_flags)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
-	struct smiapp_subdev *ssds[] = {
-		sensor->scaler,
-		sensor->binner,
-		sensor->pixel_array,
-	};
-	unsigned int i;
 	int rval;
 
-	for (i = 0; i < SMIAPP_SUBDEVS - 1; i++) {
-		struct smiapp_subdev *this = ssds[i + 1];
-		struct smiapp_subdev *last = ssds[i];
+	if (!sink_ssd)
+		return 0;
 
-		if (!last)
-			continue;
+	rval = media_entity_pads_init(&ssd->sd.entity,
+				      ssd->npads, ssd->pads);
+	if (rval) {
+		dev_err(&client->dev,
+			"media_entity_pads_init failed\n");
+		return rval;
+	}
 
-		rval = media_entity_pads_init(&this->sd.entity,
-					 this->npads, this->pads);
-		if (rval) {
-			dev_err(&client->dev,
-				"media_entity_pads_init failed\n");
-			return rval;
-		}
+	rval = v4l2_device_register_subdev(sensor->src->sd.v4l2_dev,
+					   &ssd->sd);
+	if (rval) {
+		dev_err(&client->dev,
+			"v4l2_device_register_subdev failed\n");
+		return rval;
+	}
 
-		rval = v4l2_device_register_subdev(sensor->src->sd.v4l2_dev,
-						   &this->sd);
-		if (rval) {
-			dev_err(&client->dev,
-				"v4l2_device_register_subdev failed\n");
-			return rval;
-		}
+	rval = media_create_pad_link(&ssd->sd.entity, source_pad,
+				     &sink_ssd->sd.entity, sink_pad,
+				     link_flags);
+	if (rval) {
+		dev_err(&client->dev,
+			"media_create_pad_link failed\n");
+		return rval;
+	}
 
-		rval = media_create_pad_link(&this->sd.entity,
-					     this->source_pad,
-					     &last->sd.entity,
-					     last->sink_pad,
-					     MEDIA_LNK_FL_ENABLED |
-					     MEDIA_LNK_FL_IMMUTABLE);
-		if (rval) {
-			dev_err(&client->dev,
-				"media_create_pad_link failed\n");
+	return 0;
+}
+
+static int smiapp_register_subdevs(struct smiapp_sensor *sensor)
+{
+	int rval;
+
+	if (sensor->scaler) {
+		rval = smiapp_register_subdev(
+			sensor, sensor->binner, sensor->scaler,
+			SMIAPP_PAD_SRC, SMIAPP_PAD_SINK,
+			MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
+		if (rval < 0)
 			return rval;
-		}
 	}
 
-	return 0;
+	return smiapp_register_subdev(
+		sensor, sensor->pixel_array, sensor->binner,
+		SMIAPP_PA_PAD_SRC, SMIAPP_PAD_SINK,
+		MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
 }
 
 static void smiapp_cleanup(struct smiapp_sensor *sensor)
-- 
2.1.4

Re: [PATCH v2 04/17] smiapp: Split off sub-device registration into two

[Sebastian Reichel]

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

Hi,

On Thu, Sep 15, 2016 at 02:22:18PM +0300, Sakari Ailus wrote:
> Remove the loop in sub-device registration and create each sub-device
> explicitly instead.

Reviewed-By: Sebastian Reichel <sre@kernel.org>

> +static int smiapp_register_subdevs(struct smiapp_sensor *sensor)
> +{
> +	int rval;
> +
> +	if (sensor->scaler) {
> +		rval = smiapp_register_subdev(
> +			sensor, sensor->binner, sensor->scaler,
> +			SMIAPP_PAD_SRC, SMIAPP_PAD_SINK,
> +			MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
> +		if (rval < 0)
>  			return rval;
> -		}
>  	}
>  
> -	return 0;
> +	return smiapp_register_subdev(
> +		sensor, sensor->pixel_array, sensor->binner,
> +		SMIAPP_PA_PAD_SRC, SMIAPP_PAD_SINK,
> +		MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
>  }

I haven't looked at the remaining code, but is sensor->scaler
stuff being cleaned up properly if the binner part fails?

-- Sebastian

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

Re: [PATCH v2 04/17] smiapp: Split off sub-device registration into two

[Sakari Ailus]

Hi Sebastian,

Sebastian Reichel wrote:
> Hi,
>
> On Thu, Sep 15, 2016 at 02:22:18PM +0300, Sakari Ailus wrote:
>> Remove the loop in sub-device registration and create each sub-device
>> explicitly instead.
>
> Reviewed-By: Sebastian Reichel <sre@kernel.org>

Thanks!

>
>> +static int smiapp_register_subdevs(struct smiapp_sensor *sensor)
>> +{
>> +	int rval;
>> +
>> +	if (sensor->scaler) {
>> +		rval = smiapp_register_subdev(
>> +			sensor, sensor->binner, sensor->scaler,
>> +			SMIAPP_PAD_SRC, SMIAPP_PAD_SINK,
>> +			MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
>> +		if (rval < 0)
>>   			return rval;
>> -		}
>>   	}
>>
>> -	return 0;
>> +	return smiapp_register_subdev(
>> +		sensor, sensor->pixel_array, sensor->binner,
>> +		SMIAPP_PA_PAD_SRC, SMIAPP_PAD_SINK,
>> +		MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
>>   }
>
> I haven't looked at the remaining code, but is sensor->scaler
> stuff being cleaned up properly if the binner part fails?

That's a very good question. I don't think it is. But that's how the 
code has always been --- there are issues left to be resolved if 
registered() fails for a reason or another. For instance, removing and 
reloading the omap3-isp module will cause a failure in the smiapp driver 
unless it's unloaded as well.

I think I prefer to fix that in a different patch(set) as this one is 
quite large already.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 04/17] smiapp: Split off sub-device registration into two

[Sebastian Reichel]

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

Hi,

On Mon, Sep 19, 2016 at 11:50:02PM +0300, Sakari Ailus wrote:
> Hi Sebastian,
> 
> Sebastian Reichel wrote:
> > Hi,
> > 
> > On Thu, Sep 15, 2016 at 02:22:18PM +0300, Sakari Ailus wrote:
> > > Remove the loop in sub-device registration and create each sub-device
> > > explicitly instead.
> > 
> > Reviewed-By: Sebastian Reichel <sre@kernel.org>
> 
> Thanks!
> 
> > 
> > > +static int smiapp_register_subdevs(struct smiapp_sensor *sensor)
> > > +{
> > > +	int rval;
> > > +
> > > +	if (sensor->scaler) {
> > > +		rval = smiapp_register_subdev(
> > > +			sensor, sensor->binner, sensor->scaler,
> > > +			SMIAPP_PAD_SRC, SMIAPP_PAD_SINK,
> > > +			MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
> > > +		if (rval < 0)
> > >   			return rval;
> > > -		}
> > >   	}
> > > 
> > > -	return 0;
> > > +	return smiapp_register_subdev(
> > > +		sensor, sensor->pixel_array, sensor->binner,
> > > +		SMIAPP_PA_PAD_SRC, SMIAPP_PAD_SINK,
> > > +		MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
> > >   }
> > 
> > I haven't looked at the remaining code, but is sensor->scaler
> > stuff being cleaned up properly if the binner part fails?
> 
> That's a very good question. I don't think it is. But that's how
> the code has always been 

Yes, it's not a regression introduced by this patch, that's why I
gave Reviewed-By ;)

> --- there are issues left to be resolved if registered() fails for
> a reason or another. For instance, removing and reloading the
> omap3-isp module will cause a failure in the smiapp driver unless
> it's unloaded as well.
>
> I think I prefer to fix that in a different patch(set) as this one
> is quite large already.

ok.

-- Sebastian

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

[PATCH v2 05/17] smiapp: Provide a common function to obtain native pixel array size

[Sakari Ailus]

The same pixel array size is required for the active format of each
sub-device sink pad and try format of each sink pad of each opened file
handle as well as for the native size rectangle.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 9022ffc..31d74c1 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2161,6 +2161,15 @@ static int smiapp_set_crop(struct v4l2_subdev *subdev,
 	return 0;
 }
 
+static void smiapp_get_native_size(struct smiapp_subdev *ssd,
+				    struct v4l2_rect *r)
+{
+	r->top = 0;
+	r->left = 0;
+	r->width = ssd->sensor->limits[SMIAPP_LIMIT_X_ADDR_MAX] + 1;
+	r->height = ssd->sensor->limits[SMIAPP_LIMIT_Y_ADDR_MAX] + 1;
+}
+
 static int __smiapp_get_selection(struct v4l2_subdev *subdev,
 				  struct v4l2_subdev_pad_config *cfg,
 				  struct v4l2_subdev_selection *sel)
@@ -2192,17 +2201,12 @@ static int __smiapp_get_selection(struct v4l2_subdev *subdev,
 	switch (sel->target) {
 	case V4L2_SEL_TGT_CROP_BOUNDS:
 	case V4L2_SEL_TGT_NATIVE_SIZE:
-		if (ssd == sensor->pixel_array) {
-			sel->r.left = sel->r.top = 0;
-			sel->r.width =
-				sensor->limits[SMIAPP_LIMIT_X_ADDR_MAX] + 1;
-			sel->r.height =
-				sensor->limits[SMIAPP_LIMIT_Y_ADDR_MAX] + 1;
-		} else if (sel->pad == ssd->sink_pad) {
+		if (ssd == sensor->pixel_array)
+			smiapp_get_native_size(ssd, &sel->r);
+		else if (sel->pad == ssd->sink_pad)
 			sel->r = sink_fmt;
-		} else {
+		else
 			sel->r = *comp;
-		}
 		break;
 	case V4L2_SEL_TGT_CROP:
 	case V4L2_SEL_TGT_COMPOSE_BOUNDS:
@@ -2561,10 +2565,8 @@ static void smiapp_create_subdev(struct smiapp_sensor *sensor,
 		 sizeof(ssd->sd.name), "%s %s %d-%4.4x", sensor->minfo.name,
 		 name, i2c_adapter_id(client->adapter), client->addr);
 
-	ssd->sink_fmt.width =
-		sensor->limits[SMIAPP_LIMIT_X_ADDR_MAX] + 1;
-	ssd->sink_fmt.height =
-		sensor->limits[SMIAPP_LIMIT_Y_ADDR_MAX] + 1;
+	smiapp_get_native_size(ssd, &ssd->sink_fmt);
+
 	ssd->compose.width = ssd->sink_fmt.width;
 	ssd->compose.height = ssd->sink_fmt.height;
 	ssd->crop[ssd->source_pad] = ssd->compose;
@@ -2838,16 +2840,13 @@ static int smiapp_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 		struct v4l2_rect *try_crop = v4l2_subdev_get_try_crop(sd, fh->pad, i);
 		struct v4l2_rect *try_comp;
 
-		try_fmt->width = sensor->limits[SMIAPP_LIMIT_X_ADDR_MAX] + 1;
-		try_fmt->height = sensor->limits[SMIAPP_LIMIT_Y_ADDR_MAX] + 1;
+		smiapp_get_native_size(ssd, try_crop);
+
+		try_fmt->width = try_crop->width;
+		try_fmt->height = try_crop->height;
 		try_fmt->code = mbus_code;
 		try_fmt->field = V4L2_FIELD_NONE;
 
-		try_crop->top = 0;
-		try_crop->left = 0;
-		try_crop->width = try_fmt->width;
-		try_crop->height = try_fmt->height;
-
 		if (ssd != sensor->pixel_array)
 			continue;
 
-- 
2.1.4

Re: [PATCH v2 05/17] smiapp: Provide a common function to obtain native pixel array size

[Sebastian Reichel]

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

Hi,

On Thu, Sep 15, 2016 at 02:22:19PM +0300, Sakari Ailus wrote:
> The same pixel array size is required for the active format of each
> sub-device sink pad and try format of each sink pad of each opened file
> handle as well as for the native size rectangle.

Reviewed-By: Sebastian Reichel <sre@kernel.org>

-- Sebastian

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

[PATCH v2 06/17] smiapp: Remove unnecessary BUG_ON()'s

[Sakari Ailus]

Instead, calculate how much is needed and then allocate the memory
dynamically.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 31d74c1..5d251b4 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -621,7 +621,7 @@ static int smiapp_init_controls(struct smiapp_sensor *sensor)
 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];
+		sensor->csi_format->compressed - sensor->compressed_min_bpp];
 	unsigned int max, i;
 
 	for (i = 0; i < ARRAY_SIZE(sensor->test_data); i++) {
@@ -754,6 +754,7 @@ static int smiapp_get_mbus_formats(struct smiapp_sensor *sensor)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
 	struct smiapp_pll *pll = &sensor->pll;
+	u8 compressed_max_bpp = 0;
 	unsigned int type, n;
 	unsigned int i, pixel_order;
 	int rval;
@@ -826,16 +827,27 @@ static int smiapp_get_mbus_formats(struct smiapp_sensor *sensor)
 	pll->scale_m = sensor->scale_m;
 
 	for (i = 0; i < ARRAY_SIZE(smiapp_csi_data_formats); i++) {
+		sensor->compressed_min_bpp =
+			min(smiapp_csi_data_formats[i].compressed,
+			    sensor->compressed_min_bpp);
+		compressed_max_bpp =
+			max(smiapp_csi_data_formats[i].compressed,
+			    compressed_max_bpp);
+	}
+
+	sensor->valid_link_freqs = devm_kcalloc(
+		&client->dev,
+		compressed_max_bpp - sensor->compressed_min_bpp + 1,
+		sizeof(*sensor->valid_link_freqs), GFP_KERNEL);
+
+	for (i = 0; i < ARRAY_SIZE(smiapp_csi_data_formats); i++) {
 		const struct smiapp_csi_data_format *f =
 			&smiapp_csi_data_formats[i];
 		unsigned long *valid_link_freqs =
 			&sensor->valid_link_freqs[
-				f->compressed - SMIAPP_COMPRESSED_BASE];
+				f->compressed - sensor->compressed_min_bpp];
 		unsigned int j;
 
-		BUG_ON(f->compressed < SMIAPP_COMPRESSED_BASE);
-		BUG_ON(f->compressed > SMIAPP_COMPRESSED_MAX);
-
 		if (!(sensor->default_mbus_frame_fmts & 1 << i))
 			continue;
 
@@ -1769,7 +1781,7 @@ static int smiapp_set_format_source(struct v4l2_subdev *subdev,
 
 	valid_link_freqs = 
 		&sensor->valid_link_freqs[sensor->csi_format->compressed
-					  - SMIAPP_COMPRESSED_BASE];
+					  - sensor->compressed_min_bpp];
 
 	__v4l2_ctrl_modify_range(
 		sensor->link_freq, 0,
diff --git a/drivers/media/i2c/smiapp/smiapp.h b/drivers/media/i2c/smiapp/smiapp.h
index aae72bc..e71271e 100644
--- a/drivers/media/i2c/smiapp/smiapp.h
+++ b/drivers/media/i2c/smiapp/smiapp.h
@@ -150,11 +150,6 @@ struct smiapp_csi_data_format {
 #define SMIAPP_PAD_SRC			1
 #define SMIAPP_PADS			2
 
-#define SMIAPP_COMPRESSED_BASE		8
-#define SMIAPP_COMPRESSED_MAX		16
-#define SMIAPP_NR_OF_COMPRESSED		(SMIAPP_COMPRESSED_MAX - \
-					 SMIAPP_COMPRESSED_BASE + 1)
-
 struct smiapp_binning_subtype {
 	u8 horizontal:4;
 	u8 vertical:4;
@@ -224,6 +219,7 @@ struct smiapp_sensor {
 
 	bool streaming;
 	bool dev_init_done;
+	u8 compressed_min_bpp;
 
 	u8 *nvm;		/* nvm memory buffer */
 	unsigned int nvm_size;	/* bytes */
@@ -233,7 +229,7 @@ struct smiapp_sensor {
 	struct smiapp_pll pll;
 
 	/* Is a default format supported for a given BPP? */
-	unsigned long valid_link_freqs[SMIAPP_NR_OF_COMPRESSED];
+	unsigned long *valid_link_freqs;
 
 	/* Pixel array controls */
 	struct v4l2_ctrl *analog_gain;
-- 
2.1.4

Re: [PATCH v2 06/17] smiapp: Remove unnecessary BUG_ON()'s

[Sebastian Reichel]

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

Hi,

On Thu, Sep 15, 2016 at 02:22:20PM +0300, Sakari Ailus wrote:
> Instead, calculate how much is needed and then allocate the memory
> dynamically.

Reviewed-By: Sebastian Reichel <sre@kernel.org>

-- Sebastian

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

[PATCH v2 07/17] smiapp: Always initialise the sensor in probe

[Sakari Ailus]

Initialise the sensor in probe. The reason why it wasn't previously done
in case of platform data was that the probe() of the driver that provided
the clock through the set_xclk() callback would need to finish before the
probe() function of the smiapp driver. The set_xclk() callback no longer
exists.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 5d251b4..13322f3 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2530,8 +2530,19 @@ static int smiapp_register_subdev(struct smiapp_sensor *sensor,
 	return 0;
 }
 
-static int smiapp_register_subdevs(struct smiapp_sensor *sensor)
+static void smiapp_cleanup(struct smiapp_sensor *sensor)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
+
+	device_remove_file(&client->dev, &dev_attr_nvm);
+	device_remove_file(&client->dev, &dev_attr_ident);
+
+	smiapp_free_controls(sensor);
+}
+
+static int smiapp_registered(struct v4l2_subdev *subdev)
 {
+	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
 	int rval;
 
 	if (sensor->scaler) {
@@ -2540,23 +2551,18 @@ static int smiapp_register_subdevs(struct smiapp_sensor *sensor)
 			SMIAPP_PAD_SRC, SMIAPP_PAD_SINK,
 			MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
 		if (rval < 0)
-			return rval;
+			goto out_err;
 	}
 
 	return smiapp_register_subdev(
 		sensor, sensor->pixel_array, sensor->binner,
 		SMIAPP_PA_PAD_SRC, SMIAPP_PAD_SINK,
 		MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
-}
 
-static void smiapp_cleanup(struct smiapp_sensor *sensor)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
-
-	device_remove_file(&client->dev, &dev_attr_nvm);
-	device_remove_file(&client->dev, &dev_attr_ident);
+out_err:
+	smiapp_cleanup(sensor);
 
-	smiapp_free_controls(sensor);
+	return rval;
 }
 
 static void smiapp_create_subdev(struct smiapp_sensor *sensor,
@@ -2817,25 +2823,6 @@ out_power_off:
 	return rval;
 }
 
-static int smiapp_registered(struct v4l2_subdev *subdev)
-{
-	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
-	struct i2c_client *client = v4l2_get_subdevdata(subdev);
-	int rval;
-
-	if (!client->dev.of_node) {
-		rval = smiapp_init(sensor);
-		if (rval)
-			return rval;
-	}
-
-	rval = smiapp_register_subdevs(sensor);
-	if (rval)
-		smiapp_cleanup(sensor);
-
-	return rval;
-}
-
 static int smiapp_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 {
 	struct smiapp_subdev *ssd = to_smiapp_subdev(sd);
@@ -3077,11 +3064,9 @@ static int smiapp_probe(struct i2c_client *client,
 	sensor->src->sensor = sensor;
 	sensor->src->pads[0].flags = MEDIA_PAD_FL_SOURCE;
 
-	if (client->dev.of_node) {
-		rval = smiapp_init(sensor);
-		if (rval)
-			goto out_media_entity_cleanup;
-	}
+	rval = smiapp_init(sensor);
+	if (rval)
+		goto out_media_entity_cleanup;
 
 	rval = media_entity_pads_init(&sensor->src->sd.entity, 2,
 				 sensor->src->pads);
-- 
2.1.4

Re: [PATCH v2 07/17] smiapp: Always initialise the sensor in probe

[Sebastian Reichel]

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

Hi,

On Thu, Sep 15, 2016 at 02:22:21PM +0300, Sakari Ailus wrote:
> Initialise the sensor in probe. The reason why it wasn't previously done
> in case of platform data was that the probe() of the driver that provided
> the clock through the set_xclk() callback would need to finish before the
> probe() function of the smiapp driver. The set_xclk() callback no longer
> exists.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/i2c/smiapp/smiapp-core.c | 53 ++++++++++++----------------------
>  1 file changed, 19 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
> index 5d251b4..13322f3 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -2530,8 +2530,19 @@ static int smiapp_register_subdev(struct smiapp_sensor *sensor,
>  	return 0;
>  }
>  
> -static int smiapp_register_subdevs(struct smiapp_sensor *sensor)
> +static void smiapp_cleanup(struct smiapp_sensor *sensor)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> +
> +	device_remove_file(&client->dev, &dev_attr_nvm);
> +	device_remove_file(&client->dev, &dev_attr_ident);
> +
> +	smiapp_free_controls(sensor);
> +}
> +
> +static int smiapp_registered(struct v4l2_subdev *subdev)
>  {
> +	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
>  	int rval;
>  
>  	if (sensor->scaler) {
> @@ -2540,23 +2551,18 @@ static int smiapp_register_subdevs(struct smiapp_sensor *sensor)
>  			SMIAPP_PAD_SRC, SMIAPP_PAD_SINK,
>  			MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
>  		if (rval < 0)
> -			return rval;
> +			goto out_err;
>  	}
>  
>  	return smiapp_register_subdev(
>  		sensor, sensor->pixel_array, sensor->binner,
>  		SMIAPP_PA_PAD_SRC, SMIAPP_PAD_SINK,
>  		MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);

I guess you should also handle errors from the second
smiapp_register_subdev call?

> -}
>  
> -static void smiapp_cleanup(struct smiapp_sensor *sensor)
> -{
> -	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
> -
> -	device_remove_file(&client->dev, &dev_attr_nvm);
> -	device_remove_file(&client->dev, &dev_attr_ident);
> +out_err:
> +	smiapp_cleanup(sensor);
>  
> -	smiapp_free_controls(sensor);
> +	return rval;
>  }

-- Sebastian

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

Re: [PATCH v2 07/17] smiapp: Always initialise the sensor in probe

[Sakari Ailus]

Sebastian Reichel wrote:
> Hi,
>
> On Thu, Sep 15, 2016 at 02:22:21PM +0300, Sakari Ailus wrote:
>> Initialise the sensor in probe. The reason why it wasn't previously done
>> in case of platform data was that the probe() of the driver that provided
>> the clock through the set_xclk() callback would need to finish before the
>> probe() function of the smiapp driver. The set_xclk() callback no longer
>> exists.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>>   drivers/media/i2c/smiapp/smiapp-core.c | 53 ++++++++++++----------------------
>>   1 file changed, 19 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
>> index 5d251b4..13322f3 100644
>> --- a/drivers/media/i2c/smiapp/smiapp-core.c
>> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
>> @@ -2530,8 +2530,19 @@ static int smiapp_register_subdev(struct smiapp_sensor *sensor,
>>   	return 0;
>>   }
>>
>> -static int smiapp_register_subdevs(struct smiapp_sensor *sensor)
>> +static void smiapp_cleanup(struct smiapp_sensor *sensor)
>> +{
>> +	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
>> +
>> +	device_remove_file(&client->dev, &dev_attr_nvm);
>> +	device_remove_file(&client->dev, &dev_attr_ident);
>> +
>> +	smiapp_free_controls(sensor);
>> +}
>> +
>> +static int smiapp_registered(struct v4l2_subdev *subdev)
>>   {
>> +	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
>>   	int rval;
>>
>>   	if (sensor->scaler) {
>> @@ -2540,23 +2551,18 @@ static int smiapp_register_subdevs(struct smiapp_sensor *sensor)
>>   			SMIAPP_PAD_SRC, SMIAPP_PAD_SINK,
>>   			MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
>>   		if (rval < 0)
>> -			return rval;
>> +			goto out_err;
>>   	}
>>
>>   	return smiapp_register_subdev(
>>   		sensor, sensor->pixel_array, sensor->binner,
>>   		SMIAPP_PA_PAD_SRC, SMIAPP_PAD_SINK,
>>   		MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
>
> I guess you should also handle errors from the second
> smiapp_register_subdev call?

Um, yes. Perhaps it'd be better just fix it here now that we still 
remember the problem. :-) I'll fix that for v2.

>
>> -}
>>
>> -static void smiapp_cleanup(struct smiapp_sensor *sensor)
>> -{
>> -	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
>> -
>> -	device_remove_file(&client->dev, &dev_attr_nvm);
>> -	device_remove_file(&client->dev, &dev_attr_ident);
>> +out_err:
>> +	smiapp_cleanup(sensor);
>>
>> -	smiapp_free_controls(sensor);
>> +	return rval;
>>   }
>
> -- Sebastian
>


-- 
Sakari Ailus
sakari.ailus@linux.intel.com

[PATCH v2 08/17] smiapp: Merge smiapp_init() with smiapp_probe()

[Sakari Ailus]

The smiapp_probe() is the sole caller of smiapp_init(). Unify the two.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 13322f3..0b5671c 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2605,224 +2605,6 @@ static void smiapp_create_subdev(struct smiapp_sensor *sensor,
 	v4l2_set_subdevdata(&ssd->sd, client);
 }
 
-static int smiapp_init(struct smiapp_sensor *sensor)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
-	struct smiapp_pll *pll = &sensor->pll;
-	unsigned int i;
-	int rval;
-
-	sensor->vana = devm_regulator_get(&client->dev, "vana");
-	if (IS_ERR(sensor->vana)) {
-		dev_err(&client->dev, "could not get regulator for vana\n");
-		return PTR_ERR(sensor->vana);
-	}
-
-	sensor->ext_clk = devm_clk_get(&client->dev, NULL);
-	if (IS_ERR(sensor->ext_clk)) {
-		dev_err(&client->dev, "could not get clock (%ld)\n",
-			PTR_ERR(sensor->ext_clk));
-		return -EPROBE_DEFER;
-	}
-
-	rval = clk_set_rate(sensor->ext_clk,
-			    sensor->hwcfg->ext_clk);
-	if (rval < 0) {
-		dev_err(&client->dev,
-			"unable to set clock freq to %u\n",
-			sensor->hwcfg->ext_clk);
-		return rval;
-	}
-
-	sensor->xshutdown = devm_gpiod_get_optional(&client->dev, "xshutdown",
-						    GPIOD_OUT_LOW);
-	if (IS_ERR(sensor->xshutdown))
-		return PTR_ERR(sensor->xshutdown);
-
-	rval = smiapp_power_on(sensor);
-	if (rval)
-		return -ENODEV;
-
-	rval = smiapp_identify_module(sensor);
-	if (rval) {
-		rval = -ENODEV;
-		goto out_power_off;
-	}
-
-	rval = smiapp_get_all_limits(sensor);
-	if (rval) {
-		rval = -ENODEV;
-		goto out_power_off;
-	}
-
-	/*
-	 * Handle Sensor Module orientation on the board.
-	 *
-	 * The application of H-FLIP and V-FLIP on the sensor is modified by
-	 * the sensor orientation on the board.
-	 *
-	 * For SMIAPP_BOARD_SENSOR_ORIENT_180 the default behaviour is to set
-	 * both H-FLIP and V-FLIP for normal operation which also implies
-	 * that a set/unset operation for user space HFLIP and VFLIP v4l2
-	 * controls will need to be internally inverted.
-	 *
-	 * Rotation also changes the bayer pattern.
-	 */
-	if (sensor->hwcfg->module_board_orient ==
-	    SMIAPP_MODULE_BOARD_ORIENT_180)
-		sensor->hvflip_inv_mask = SMIAPP_IMAGE_ORIENTATION_HFLIP |
-					  SMIAPP_IMAGE_ORIENTATION_VFLIP;
-
-	rval = smiapp_call_quirk(sensor, limits);
-	if (rval) {
-		dev_err(&client->dev, "limits quirks failed\n");
-		goto out_power_off;
-	}
-
-	if (sensor->limits[SMIAPP_LIMIT_BINNING_CAPABILITY]) {
-		u32 val;
-
-		rval = smiapp_read(sensor,
-				   SMIAPP_REG_U8_BINNING_SUBTYPES, &val);
-		if (rval < 0) {
-			rval = -ENODEV;
-			goto out_power_off;
-		}
-		sensor->nbinning_subtypes = min_t(u8, val,
-						  SMIAPP_BINNING_SUBTYPES);
-
-		for (i = 0; i < sensor->nbinning_subtypes; i++) {
-			rval = smiapp_read(
-				sensor, SMIAPP_REG_U8_BINNING_TYPE_n(i), &val);
-			if (rval < 0) {
-				rval = -ENODEV;
-				goto out_power_off;
-			}
-			sensor->binning_subtypes[i] =
-				*(struct smiapp_binning_subtype *)&val;
-
-			dev_dbg(&client->dev, "binning %xx%x\n",
-				sensor->binning_subtypes[i].horizontal,
-				sensor->binning_subtypes[i].vertical);
-		}
-	}
-	sensor->binning_horizontal = 1;
-	sensor->binning_vertical = 1;
-
-	if (device_create_file(&client->dev, &dev_attr_ident) != 0) {
-		dev_err(&client->dev, "sysfs ident entry creation failed\n");
-		rval = -ENOENT;
-		goto out_power_off;
-	}
-	/* SMIA++ NVM initialization - it will be read from the sensor
-	 * when it is first requested by userspace.
-	 */
-	if (sensor->minfo.smiapp_version && sensor->hwcfg->nvm_size) {
-		sensor->nvm = devm_kzalloc(&client->dev,
-				sensor->hwcfg->nvm_size, GFP_KERNEL);
-		if (sensor->nvm == NULL) {
-			dev_err(&client->dev, "nvm buf allocation failed\n");
-			rval = -ENOMEM;
-			goto out_cleanup;
-		}
-
-		if (device_create_file(&client->dev, &dev_attr_nvm) != 0) {
-			dev_err(&client->dev, "sysfs nvm entry failed\n");
-			rval = -EBUSY;
-			goto out_cleanup;
-		}
-	}
-
-	/* We consider this as profile 0 sensor if any of these are zero. */
-	if (!sensor->limits[SMIAPP_LIMIT_MIN_OP_SYS_CLK_DIV] ||
-	    !sensor->limits[SMIAPP_LIMIT_MAX_OP_SYS_CLK_DIV] ||
-	    !sensor->limits[SMIAPP_LIMIT_MIN_OP_PIX_CLK_DIV] ||
-	    !sensor->limits[SMIAPP_LIMIT_MAX_OP_PIX_CLK_DIV]) {
-		sensor->minfo.smiapp_profile = SMIAPP_PROFILE_0;
-	} else if (sensor->limits[SMIAPP_LIMIT_SCALING_CAPABILITY]
-		   != SMIAPP_SCALING_CAPABILITY_NONE) {
-		if (sensor->limits[SMIAPP_LIMIT_SCALING_CAPABILITY]
-		    == SMIAPP_SCALING_CAPABILITY_HORIZONTAL)
-			sensor->minfo.smiapp_profile = SMIAPP_PROFILE_1;
-		else
-			sensor->minfo.smiapp_profile = SMIAPP_PROFILE_2;
-		sensor->scaler = &sensor->ssds[sensor->ssds_used];
-		sensor->ssds_used++;
-	} else if (sensor->limits[SMIAPP_LIMIT_DIGITAL_CROP_CAPABILITY]
-		   == SMIAPP_DIGITAL_CROP_CAPABILITY_INPUT_CROP) {
-		sensor->scaler = &sensor->ssds[sensor->ssds_used];
-		sensor->ssds_used++;
-	}
-	sensor->binner = &sensor->ssds[sensor->ssds_used];
-	sensor->ssds_used++;
-	sensor->pixel_array = &sensor->ssds[sensor->ssds_used];
-	sensor->ssds_used++;
-
-	sensor->scale_m = sensor->limits[SMIAPP_LIMIT_SCALER_N_MIN];
-
-	/* prepare PLL configuration input values */
-	pll->bus_type = SMIAPP_PLL_BUS_TYPE_CSI2;
-	pll->csi2.lanes = sensor->hwcfg->lanes;
-	pll->ext_clk_freq_hz = sensor->hwcfg->ext_clk;
-	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)
-		pll->flags |= SMIAPP_PLL_FLAG_NO_OP_CLOCKS;
-
-	if (sensor->scaler)
-		smiapp_create_subdev(sensor, sensor->scaler, "scaler", 2);
-	smiapp_create_subdev(sensor, sensor->binner, "binner", 2);
-	smiapp_create_subdev(sensor, sensor->pixel_array, "pixel_array", 1);
-
-	dev_dbg(&client->dev, "profile %d\n", sensor->minfo.smiapp_profile);
-
-	sensor->pixel_array->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
-
-	/* final steps */
-	smiapp_read_frame_fmt(sensor);
-	rval = smiapp_init_controls(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;
-		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);
-	if (rval) {
-		dev_err(&client->dev, "update mode failed\n");
-		goto out_cleanup;
-	}
-
-	sensor->streaming = false;
-	sensor->dev_init_done = true;
-
-	smiapp_power_off(sensor);
-
-	return 0;
-
-out_cleanup:
-	smiapp_cleanup(sensor);
-
-out_power_off:
-	smiapp_power_off(sensor);
-	return rval;
-}
-
 static int smiapp_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 {
 	struct smiapp_subdev *ssd = to_smiapp_subdev(sd);
@@ -3044,6 +2826,7 @@ static int smiapp_probe(struct i2c_client *client,
 {
 	struct smiapp_sensor *sensor;
 	struct smiapp_hwconfig *hwcfg = smiapp_get_hwconfig(&client->dev);
+	unsigned int i;
 	int rval;
 
 	if (hwcfg == NULL)
@@ -3064,9 +2847,206 @@ static int smiapp_probe(struct i2c_client *client,
 	sensor->src->sensor = sensor;
 	sensor->src->pads[0].flags = MEDIA_PAD_FL_SOURCE;
 
-	rval = smiapp_init(sensor);
+	sensor->vana = devm_regulator_get(&client->dev, "vana");
+	if (IS_ERR(sensor->vana)) {
+		dev_err(&client->dev, "could not get regulator for vana\n");
+		return PTR_ERR(sensor->vana);
+	}
+
+	sensor->ext_clk = devm_clk_get(&client->dev, NULL);
+	if (IS_ERR(sensor->ext_clk)) {
+		dev_err(&client->dev, "could not get clock (%ld)\n",
+			PTR_ERR(sensor->ext_clk));
+		return -EPROBE_DEFER;
+	}
+
+	rval = clk_set_rate(sensor->ext_clk,
+			    sensor->hwcfg->ext_clk);
+	if (rval < 0) {
+		dev_err(&client->dev,
+			"unable to set clock freq to %u\n",
+			sensor->hwcfg->ext_clk);
+		return rval;
+	}
+
+	sensor->xshutdown = devm_gpiod_get_optional(&client->dev, "xshutdown",
+						    GPIOD_OUT_LOW);
+	if (IS_ERR(sensor->xshutdown))
+		return PTR_ERR(sensor->xshutdown);
+
+	rval = smiapp_power_on(sensor);
 	if (rval)
-		goto out_media_entity_cleanup;
+		return -ENODEV;
+
+	rval = smiapp_identify_module(sensor);
+	if (rval) {
+		rval = -ENODEV;
+		goto out_power_off;
+	}
+
+	rval = smiapp_get_all_limits(sensor);
+	if (rval) {
+		rval = -ENODEV;
+		goto out_power_off;
+	}
+
+	/*
+	 * Handle Sensor Module orientation on the board.
+	 *
+	 * The application of H-FLIP and V-FLIP on the sensor is modified by
+	 * the sensor orientation on the board.
+	 *
+	 * For SMIAPP_BOARD_SENSOR_ORIENT_180 the default behaviour is to set
+	 * both H-FLIP and V-FLIP for normal operation which also implies
+	 * that a set/unset operation for user space HFLIP and VFLIP v4l2
+	 * controls will need to be internally inverted.
+	 *
+	 * Rotation also changes the bayer pattern.
+	 */
+	if (sensor->hwcfg->module_board_orient ==
+	    SMIAPP_MODULE_BOARD_ORIENT_180)
+		sensor->hvflip_inv_mask = SMIAPP_IMAGE_ORIENTATION_HFLIP |
+					  SMIAPP_IMAGE_ORIENTATION_VFLIP;
+
+	rval = smiapp_call_quirk(sensor, limits);
+	if (rval) {
+		dev_err(&client->dev, "limits quirks failed\n");
+		goto out_power_off;
+	}
+
+	if (sensor->limits[SMIAPP_LIMIT_BINNING_CAPABILITY]) {
+		u32 val;
+
+		rval = smiapp_read(sensor,
+				   SMIAPP_REG_U8_BINNING_SUBTYPES, &val);
+		if (rval < 0) {
+			rval = -ENODEV;
+			goto out_power_off;
+		}
+		sensor->nbinning_subtypes = min_t(u8, val,
+						  SMIAPP_BINNING_SUBTYPES);
+
+		for (i = 0; i < sensor->nbinning_subtypes; i++) {
+			rval = smiapp_read(
+				sensor, SMIAPP_REG_U8_BINNING_TYPE_n(i), &val);
+			if (rval < 0) {
+				rval = -ENODEV;
+				goto out_power_off;
+			}
+			sensor->binning_subtypes[i] =
+				*(struct smiapp_binning_subtype *)&val;
+
+			dev_dbg(&client->dev, "binning %xx%x\n",
+				sensor->binning_subtypes[i].horizontal,
+				sensor->binning_subtypes[i].vertical);
+		}
+	}
+	sensor->binning_horizontal = 1;
+	sensor->binning_vertical = 1;
+
+	if (device_create_file(&client->dev, &dev_attr_ident) != 0) {
+		dev_err(&client->dev, "sysfs ident entry creation failed\n");
+		rval = -ENOENT;
+		goto out_power_off;
+	}
+	/* SMIA++ NVM initialization - it will be read from the sensor
+	 * when it is first requested by userspace.
+	 */
+	if (sensor->minfo.smiapp_version && sensor->hwcfg->nvm_size) {
+		sensor->nvm = devm_kzalloc(&client->dev,
+				sensor->hwcfg->nvm_size, GFP_KERNEL);
+		if (sensor->nvm == NULL) {
+			dev_err(&client->dev, "nvm buf allocation failed\n");
+			rval = -ENOMEM;
+			goto out_cleanup;
+		}
+
+		if (device_create_file(&client->dev, &dev_attr_nvm) != 0) {
+			dev_err(&client->dev, "sysfs nvm entry failed\n");
+			rval = -EBUSY;
+			goto out_cleanup;
+		}
+	}
+
+	/* We consider this as profile 0 sensor if any of these are zero. */
+	if (!sensor->limits[SMIAPP_LIMIT_MIN_OP_SYS_CLK_DIV] ||
+	    !sensor->limits[SMIAPP_LIMIT_MAX_OP_SYS_CLK_DIV] ||
+	    !sensor->limits[SMIAPP_LIMIT_MIN_OP_PIX_CLK_DIV] ||
+	    !sensor->limits[SMIAPP_LIMIT_MAX_OP_PIX_CLK_DIV]) {
+		sensor->minfo.smiapp_profile = SMIAPP_PROFILE_0;
+	} else if (sensor->limits[SMIAPP_LIMIT_SCALING_CAPABILITY]
+		   != SMIAPP_SCALING_CAPABILITY_NONE) {
+		if (sensor->limits[SMIAPP_LIMIT_SCALING_CAPABILITY]
+		    == SMIAPP_SCALING_CAPABILITY_HORIZONTAL)
+			sensor->minfo.smiapp_profile = SMIAPP_PROFILE_1;
+		else
+			sensor->minfo.smiapp_profile = SMIAPP_PROFILE_2;
+		sensor->scaler = &sensor->ssds[sensor->ssds_used];
+		sensor->ssds_used++;
+	} else if (sensor->limits[SMIAPP_LIMIT_DIGITAL_CROP_CAPABILITY]
+		   == SMIAPP_DIGITAL_CROP_CAPABILITY_INPUT_CROP) {
+		sensor->scaler = &sensor->ssds[sensor->ssds_used];
+		sensor->ssds_used++;
+	}
+	sensor->binner = &sensor->ssds[sensor->ssds_used];
+	sensor->ssds_used++;
+	sensor->pixel_array = &sensor->ssds[sensor->ssds_used];
+	sensor->ssds_used++;
+
+	sensor->scale_m = sensor->limits[SMIAPP_LIMIT_SCALER_N_MIN];
+
+	/* prepare PLL configuration input values */
+	sensor->pll.bus_type = SMIAPP_PLL_BUS_TYPE_CSI2;
+	sensor->pll.csi2.lanes = sensor->hwcfg->lanes;
+	sensor->pll.ext_clk_freq_hz = sensor->hwcfg->ext_clk;
+	sensor->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)
+		sensor->pll.flags |= SMIAPP_PLL_FLAG_NO_OP_CLOCKS;
+
+	if (sensor->scaler)
+		smiapp_create_subdev(sensor, sensor->scaler, "scaler", 2);
+	smiapp_create_subdev(sensor, sensor->binner, "binner", 2);
+	smiapp_create_subdev(sensor, sensor->pixel_array, "pixel_array", 1);
+
+	dev_dbg(&client->dev, "profile %d\n", sensor->minfo.smiapp_profile);
+
+	sensor->pixel_array->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+
+	/* final steps */
+	smiapp_read_frame_fmt(sensor);
+	rval = smiapp_init_controls(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;
+		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);
+	if (rval) {
+		dev_err(&client->dev, "update mode failed\n");
+		goto out_cleanup;
+	}
+
+	sensor->streaming = false;
+	sensor->dev_init_done = true;
+
+	smiapp_power_off(sensor);
 
 	rval = media_entity_pads_init(&sensor->src->sd.entity, 2,
 				 sensor->src->pads);
@@ -3082,6 +3062,11 @@ static int smiapp_probe(struct i2c_client *client,
 out_media_entity_cleanup:
 	media_entity_cleanup(&sensor->src->sd.entity);
 
+out_cleanup:
+	smiapp_cleanup(sensor);
+
+out_power_off:
+	smiapp_power_off(sensor);
 	return rval;
 }
 
-- 
2.1.4

Re: [PATCH v2 08/17] smiapp: Merge smiapp_init() with smiapp_probe()

[Sebastian Reichel]

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

Hi,

On Thu, Sep 15, 2016 at 02:22:22PM +0300, Sakari Ailus wrote:
> The smiapp_probe() is the sole caller of smiapp_init(). Unify the two.

Reviewed-By: Sebastian Reichel <sre@kernel.org>

-- Sebastian

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

[PATCH v2 09/17] smiapp: Read frame format earlier

[Sakari Ailus]

The information gathered during frame format reading will be required
earlier in the initialisation when it was available. Also return an error
if frame format cannot be obtained.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 0b5671c..c9aee83 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2890,6 +2890,12 @@ static int smiapp_probe(struct i2c_client *client,
 		goto out_power_off;
 	}
 
+	rval = smiapp_read_frame_fmt(sensor);
+	if (rval) {
+		rval = -ENODEV;
+		goto out_power_off;
+	}
+
 	/*
 	 * Handle Sensor Module orientation on the board.
 	 *
@@ -3013,8 +3019,6 @@ static int smiapp_probe(struct i2c_client *client,
 
 	sensor->pixel_array->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
 
-	/* final steps */
-	smiapp_read_frame_fmt(sensor);
 	rval = smiapp_init_controls(sensor);
 	if (rval < 0)
 		goto out_cleanup;
-- 
2.1.4

Re: [PATCH v2 09/17] smiapp: Read frame format earlier

[Sebastian Reichel]

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

Hi,

On Thu, Sep 15, 2016 at 02:22:23PM +0300, Sakari Ailus wrote:
> The information gathered during frame format reading will be required
> earlier in the initialisation when it was available. Also return an error
> if frame format cannot be obtained.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/i2c/smiapp/smiapp-core.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
> index 0b5671c..c9aee83 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -2890,6 +2890,12 @@ static int smiapp_probe(struct i2c_client *client,
>  		goto out_power_off;
>  	}
>  
> +	rval = smiapp_read_frame_fmt(sensor);
> +	if (rval) {
> +		rval = -ENODEV;
> +		goto out_power_off;
> +	}
> +
>  	/*
>  	 * Handle Sensor Module orientation on the board.
>  	 *
> @@ -3013,8 +3019,6 @@ static int smiapp_probe(struct i2c_client *client,
>  
>  	sensor->pixel_array->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>  
> -	/* final steps */
> -	smiapp_read_frame_fmt(sensor);
>  	rval = smiapp_init_controls(sensor);
>  	if (rval < 0)
>  		goto out_cleanup;

Is this missing a Fixes tag, or will it only be required earlier for
future patches?

-- Sebastian

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

Re: [PATCH v2 09/17] smiapp: Read frame format earlier

[Sakari Ailus]

Hi Sebastian,

Sebastian Reichel wrote:
> Hi,
>
> On Thu, Sep 15, 2016 at 02:22:23PM +0300, Sakari Ailus wrote:
>> The information gathered during frame format reading will be required
>> earlier in the initialisation when it was available. Also return an error
>> if frame format cannot be obtained.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>> ---
>>   drivers/media/i2c/smiapp/smiapp-core.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
>> index 0b5671c..c9aee83 100644
>> --- a/drivers/media/i2c/smiapp/smiapp-core.c
>> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
>> @@ -2890,6 +2890,12 @@ static int smiapp_probe(struct i2c_client *client,
>>   		goto out_power_off;
>>   	}
>>
>> +	rval = smiapp_read_frame_fmt(sensor);
>> +	if (rval) {
>> +		rval = -ENODEV;
>> +		goto out_power_off;
>> +	}
>> +
>>   	/*
>>   	 * Handle Sensor Module orientation on the board.
>>   	 *
>> @@ -3013,8 +3019,6 @@ static int smiapp_probe(struct i2c_client *client,
>>
>>   	sensor->pixel_array->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>>
>> -	/* final steps */
>> -	smiapp_read_frame_fmt(sensor);
>>   	rval = smiapp_init_controls(sensor);
>>   	if (rval < 0)
>>   		goto out_cleanup;
>
> Is this missing a Fixes tag, or will it only be required earlier for
> future patches?

It's primarily for future patches. Reading the frame format will require 
limits but hardly any other information. On the other hand, the frame 
format will very likely be needed elsewhere, hence the move.

The missing return value check is just a bug which I believe has been 
there since around 2011.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 09/17] smiapp: Read frame format earlier

[Sebastian Reichel]

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

Hi,

On Tue, Sep 20, 2016 at 12:19:54AM +0300, Sakari Ailus wrote:
> Hi Sebastian,
> 
> Sebastian Reichel wrote:
> > Hi,
> > 
> > On Thu, Sep 15, 2016 at 02:22:23PM +0300, Sakari Ailus wrote:
> > > The information gathered during frame format reading will be required
> > > earlier in the initialisation when it was available. Also return an error
> > > if frame format cannot be obtained.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >   drivers/media/i2c/smiapp/smiapp-core.c | 8 ++++++--
> > >   1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
> > > index 0b5671c..c9aee83 100644
> > > --- a/drivers/media/i2c/smiapp/smiapp-core.c
> > > +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> > > @@ -2890,6 +2890,12 @@ static int smiapp_probe(struct i2c_client *client,
> > >   		goto out_power_off;
> > >   	}
> > > 
> > > +	rval = smiapp_read_frame_fmt(sensor);
> > > +	if (rval) {
> > > +		rval = -ENODEV;
> > > +		goto out_power_off;
> > > +	}
> > > +
> > >   	/*
> > >   	 * Handle Sensor Module orientation on the board.
> > >   	 *
> > > @@ -3013,8 +3019,6 @@ static int smiapp_probe(struct i2c_client *client,
> > > 
> > >   	sensor->pixel_array->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > > 
> > > -	/* final steps */
> > > -	smiapp_read_frame_fmt(sensor);
> > >   	rval = smiapp_init_controls(sensor);
> > >   	if (rval < 0)
> > >   		goto out_cleanup;
> > 
> > Is this missing a Fixes tag, or will it only be required earlier for
> > future patches?
> 
> It's primarily for future patches. Reading the frame format will require
> limits but hardly any other information. On the other hand, the frame format
> will very likely be needed elsewhere, hence the move.
> 
> The missing return value check is just a bug which I believe has been there
> since around 2011.

ok, so the move is for future patches. Then

Reviewed-By: Sebastian Reichel <sre@kernel.org>

-- Sebastian

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

[PATCH v2 10/17] smiapp: Unify setting up sub-devices

[Sakari Ailus]

The initialisation of the source sub-device is somewhat different as it's
not created by the smiapp driver itself. Remove redundancy in initialising
the two kind of sub-devices.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index c9aee83..b446d0a 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2574,6 +2574,7 @@ static void smiapp_create_subdev(struct smiapp_sensor *sensor,
 	if (ssd != sensor->src)
 		v4l2_subdev_init(&ssd->sd, &smiapp_ops);
 
+	ssd->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 	ssd->sensor = sensor;
 
 	ssd->npads = num_pads;
@@ -2599,7 +2600,6 @@ static void smiapp_create_subdev(struct smiapp_sensor *sensor,
 	if (ssd == sensor->src)
 		return;
 
-	ssd->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 	ssd->sd.internal_ops = &smiapp_internal_ops;
 	ssd->sd.owner = THIS_MODULE;
 	v4l2_set_subdevdata(&ssd->sd, client);
@@ -2843,9 +2843,6 @@ static int smiapp_probe(struct i2c_client *client,
 
 	v4l2_i2c_subdev_init(&sensor->src->sd, client, &smiapp_ops);
 	sensor->src->sd.internal_ops = &smiapp_internal_src_ops;
-	sensor->src->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
-	sensor->src->sensor = sensor;
-	sensor->src->pads[0].flags = MEDIA_PAD_FL_SOURCE;
 
 	sensor->vana = devm_regulator_get(&client->dev, "vana");
 	if (IS_ERR(sensor->vana)) {
-- 
2.1.4

Re: [PATCH v2 10/17] smiapp: Unify setting up sub-devices

[Sebastian Reichel]

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

Hi,

On Thu, Sep 15, 2016 at 02:22:24PM +0300, Sakari Ailus wrote:
> The initialisation of the source sub-device is somewhat different as it's
> not created by the smiapp driver itself. Remove redundancy in initialising
> the two kind of sub-devices.

Reviewed-By: Sebastian Reichel <sre@kernel.org>

-- Sebastian

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

[PATCH v2 11/17] smiapp: Use SMIAPP_PADS when referring to number of pads

[Sakari Ailus]

Replace plain value 2 with SMIAPP_PADS when referring to the number of
pads.

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

diff --git a/drivers/media/i2c/smiapp/smiapp.h b/drivers/media/i2c/smiapp/smiapp.h
index e71271e..f9febe0 100644
--- a/drivers/media/i2c/smiapp/smiapp.h
+++ b/drivers/media/i2c/smiapp/smiapp.h
@@ -157,9 +157,9 @@ struct smiapp_binning_subtype {
 
 struct smiapp_subdev {
 	struct v4l2_subdev sd;
-	struct media_pad pads[2];
+	struct media_pad pads[SMIAPP_PADS];
 	struct v4l2_rect sink_fmt;
-	struct v4l2_rect crop[2];
+	struct v4l2_rect crop[SMIAPP_PADS];
 	struct v4l2_rect compose; /* compose on sink */
 	unsigned short sink_pad;
 	unsigned short source_pad;
-- 
2.1.4

Re: [PATCH v2 11/17] smiapp: Use SMIAPP_PADS when referring to number of pads

[Sebastian Reichel]

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

Hi,

On Thu, Sep 15, 2016 at 02:22:25PM +0300, Sakari Ailus wrote:
> Replace plain value 2 with SMIAPP_PADS when referring to the number of
> pads.

Reviewed-By: Sebastian Reichel <sre@kernel.org>

-- Sebastian

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

[PATCH v2 12/17] smiapp: Obtain frame layout from the frame descriptor

[Sakari Ailus]

Besides the image data, SMIA++ compliant sensors also provide embedded
data in form of registers used to capture the image. Store this
information for later use in frame descriptor and routing.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index b446d0a..f09665d 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -68,10 +68,9 @@ static int smiapp_read_frame_fmt(struct smiapp_sensor *sensor)
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
 	u32 fmt_model_type, fmt_model_subtype, ncol_desc, nrow_desc;
 	unsigned int i;
-	int rval;
+	int pixel_count = 0;
 	int line_count = 0;
-	int embedded_start = -1, embedded_end = -1;
-	int image_start = 0;
+	int rval;
 
 	rval = smiapp_read(sensor, SMIAPP_REG_U8_FRAME_FORMAT_MODEL_TYPE,
 			   &fmt_model_type);
@@ -166,33 +165,36 @@ static int smiapp_read_frame_fmt(struct smiapp_sensor *sensor)
 		dev_dbg(&client->dev, "%s pixels: %d %s\n",
 			what, pixels, which);
 
-		if (i < ncol_desc)
+		if (i < ncol_desc) {
+			if (pixelcode ==
+			    SMIAPP_FRAME_FORMAT_DESC_PIXELCODE_VISIBLE)
+				sensor->visible_pixel_start = pixel_count;
+			pixel_count += pixels;
 			continue;
+		}
 
 		/* Handle row descriptors */
-		if (pixelcode
-		    == SMIAPP_FRAME_FORMAT_DESC_PIXELCODE_EMBEDDED) {
-			embedded_start = line_count;
-		} else {
-			if (pixelcode == SMIAPP_FRAME_FORMAT_DESC_PIXELCODE_VISIBLE
-			    || pixels >= sensor->limits[SMIAPP_LIMIT_MIN_FRAME_LENGTH_LINES] / 2)
-				image_start = line_count;
-			if (embedded_start != -1 && embedded_end == -1)
-				embedded_end = line_count;
+		switch (pixelcode) {
+		case SMIAPP_FRAME_FORMAT_DESC_PIXELCODE_EMBEDDED:
+			if (sensor->embedded_end)
+				break;
+			sensor->embedded_start = line_count;
+			sensor->embedded_end = line_count + pixels;
+			break;
+		case SMIAPP_FRAME_FORMAT_DESC_PIXELCODE_VISIBLE:
+			sensor->image_start = line_count;
+			break;
 		}
 		line_count += pixels;
 	}
 
-	if (embedded_start == -1 || embedded_end == -1) {
-		embedded_start = 0;
-		embedded_end = 0;
-	}
-
-	sensor->image_start = image_start;
+	if (sensor->embedded_end > sensor->image_start)
+		sensor->image_start = sensor->embedded_end;
 
 	dev_dbg(&client->dev, "embedded data from lines %d to %d\n",
-		embedded_start, embedded_end);
-	dev_dbg(&client->dev, "image data starts at line %d\n", image_start);
+		sensor->embedded_start, sensor->embedded_end);
+	dev_dbg(&client->dev, "image data starts at line %d\n",
+		sensor->image_start);
 
 	return 0;
 }
diff --git a/drivers/media/i2c/smiapp/smiapp.h b/drivers/media/i2c/smiapp/smiapp.h
index f9febe0..d7b52a6 100644
--- a/drivers/media/i2c/smiapp/smiapp.h
+++ b/drivers/media/i2c/smiapp/smiapp.h
@@ -213,7 +213,10 @@ struct smiapp_sensor {
 
 	u8 hvflip_inv_mask; /* H/VFLIP inversion due to sensor orientation */
 	u8 frame_skip;
-	u16 image_start;	/* Offset to first line after metadata lines */
+	u16 embedded_start; /* embedded data start line */
+	u16 embedded_end;
+	u16 image_start; /* image data start line */
+	u16 visible_pixel_start; /* start pixel of the visible image */
 
 	int power_count;
 
-- 
2.1.4

Re: [PATCH v2 12/17] smiapp: Obtain frame layout from the frame descriptor

[Sebastian Reichel]

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

Hi,

On Thu, Sep 15, 2016 at 02:22:26PM +0300, Sakari Ailus wrote:
> Besides the image data, SMIA++ compliant sensors also provide embedded
> data in form of registers used to capture the image. Store this
> information for later use in frame descriptor and routing.

Reviewed-By: Sebastian Reichel <sre@kernel.org>

> [...]
>
> +	if (sensor->embedded_end > sensor->image_start)
> +		sensor->image_start = sensor->embedded_end;

Maybe add a dev_dbg about sensor format information being broken?

-- Sebastian

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

[PATCH v2 13/17] smiapp: Improve debug messages from frame layout reading

[Sakari Ailus]

Provide more debugging information on reading the frame layout.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index f09665d..8b042e2 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -100,12 +100,11 @@ static int smiapp_read_frame_fmt(struct smiapp_sensor *sensor)
 		u32 pixels;
 		char *which;
 		char *what;
+		u32 reg;
 
 		if (fmt_model_type == SMIAPP_FRAME_FORMAT_MODEL_TYPE_2BYTE) {
-			rval = smiapp_read(
-				sensor,
-				SMIAPP_REG_U16_FRAME_FORMAT_DESCRIPTOR_2(i),
-				&desc);
+			reg = SMIAPP_REG_U16_FRAME_FORMAT_DESCRIPTOR_2(i);
+			rval = smiapp_read(sensor, reg,	&desc);
 			if (rval)
 				return rval;
 
@@ -116,10 +115,8 @@ static int smiapp_read_frame_fmt(struct smiapp_sensor *sensor)
 			pixels = desc & SMIAPP_FRAME_FORMAT_DESC_2_PIXELS_MASK;
 		} else if (fmt_model_type
 			   == SMIAPP_FRAME_FORMAT_MODEL_TYPE_4BYTE) {
-			rval = smiapp_read(
-				sensor,
-				SMIAPP_REG_U32_FRAME_FORMAT_DESCRIPTOR_4(i),
-				&desc);
+			reg = SMIAPP_REG_U32_FRAME_FORMAT_DESCRIPTOR_4(i);
+			rval = smiapp_read(sensor, reg, &desc);
 			if (rval)
 				return rval;
 
@@ -130,7 +127,7 @@ static int smiapp_read_frame_fmt(struct smiapp_sensor *sensor)
 			pixels = desc & SMIAPP_FRAME_FORMAT_DESC_4_PIXELS_MASK;
 		} else {
 			dev_dbg(&client->dev,
-				"invalid frame format model type %d\n",
+				"0x8.8x invalid frame format model type %d\n",
 				fmt_model_type);
 			return -EINVAL;
 		}
@@ -158,12 +155,12 @@ static int smiapp_read_frame_fmt(struct smiapp_sensor *sensor)
 			break;
 		default:
 			what = "invalid";
-			dev_dbg(&client->dev, "pixelcode %d\n", pixelcode);
 			break;
 		}
 
-		dev_dbg(&client->dev, "%s pixels: %d %s\n",
-			what, pixels, which);
+		dev_dbg(&client->dev,
+			"0x%8.8x %s pixels: %d %s (pixelcode %u)\n", reg,
+			what, pixels, which, pixelcode);
 
 		if (i < ncol_desc) {
 			if (pixelcode ==
-- 
2.1.4

Re: [PATCH v2 13/17] smiapp: Improve debug messages from frame layout reading

[Sebastian Reichel]

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

Hi,

On Thu, Sep 15, 2016 at 02:22:27PM +0300, Sakari Ailus wrote:
> Provide more debugging information on reading the frame layout.
> 
> [...]
>
> @@ -130,7 +127,7 @@ static int smiapp_read_frame_fmt(struct smiapp_sensor *sensor)
>  			pixels = desc & SMIAPP_FRAME_FORMAT_DESC_4_PIXELS_MASK;
>  		} else {
>  			dev_dbg(&client->dev,
> -				"invalid frame format model type %d\n",
> +				"0x8.8x invalid frame format model type %d\n",

0x8.8x doesn't look very interesting ;)

Apart from the '%' the actual argument is also missing.

> [...]

Once that is fixed:

Reviewed-By: Sebastian Reichel <sre@kernel.org>

-- Sebastian

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

[PATCH v2 14/17] smiapp: Remove useless newlines and other small cleanups

[Sakari Ailus]

The code probably has been unindented at some point but rewrapping has not
been done. Do it now.

Also remove a useless memory allocation failure message.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 8b042e2..3548225 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -442,8 +442,7 @@ static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
 			orient |= SMIAPP_IMAGE_ORIENTATION_VFLIP;
 
 		orient ^= sensor->hvflip_inv_mask;
-		rval = smiapp_write(sensor,
-				    SMIAPP_REG_U8_IMAGE_ORIENTATION,
+		rval = smiapp_write(sensor, SMIAPP_REG_U8_IMAGE_ORIENTATION,
 				    orient);
 		if (rval < 0)
 			return rval;
@@ -458,10 +457,8 @@ static int smiapp_set_ctrl(struct v4l2_ctrl *ctrl)
 		__smiapp_update_exposure_limits(sensor);
 
 		if (exposure > sensor->exposure->maximum) {
-			sensor->exposure->val =
-				sensor->exposure->maximum;
-			rval = smiapp_set_ctrl(
-				sensor->exposure);
+			sensor->exposure->val =	sensor->exposure->maximum;
+			rval = smiapp_set_ctrl(sensor->exposure);
 			if (rval < 0)
 				return rval;
 		}
@@ -1318,8 +1315,7 @@ static int smiapp_power_on(struct smiapp_sensor *sensor)
 	if (!sensor->pixel_array)
 		return 0;
 
-	rval = v4l2_ctrl_handler_setup(
-		&sensor->pixel_array->ctrl_handler);
+	rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->ctrl_handler);
 	if (rval)
 		goto out_cci_addr_fail;
 
@@ -1625,7 +1621,8 @@ static int __smiapp_get_format(struct v4l2_subdev *subdev,
 	struct smiapp_subdev *ssd = to_smiapp_subdev(subdev);
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
-		fmt->format = *v4l2_subdev_get_try_format(subdev, cfg, fmt->pad);
+		fmt->format = *v4l2_subdev_get_try_format(subdev, cfg,
+							  fmt->pad);
 	} else {
 		struct v4l2_rect *r;
 
@@ -1725,7 +1722,6 @@ static void smiapp_propagate(struct v4l2_subdev *subdev,
 static const struct smiapp_csi_data_format
 *smiapp_validate_csi_data_format(struct smiapp_sensor *sensor, u32 code)
 {
-	const struct smiapp_csi_data_format *csi_format = sensor->csi_format;
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(smiapp_csi_data_formats); i++) {
@@ -1734,7 +1730,7 @@ static const struct smiapp_csi_data_format
 			return &smiapp_csi_data_formats[i];
 	}
 
-	return csi_format;
+	return sensor->csi_format;
 }
 
 static int smiapp_set_format_source(struct v4l2_subdev *subdev,
@@ -2068,8 +2064,7 @@ static int smiapp_set_compose(struct v4l2_subdev *subdev,
 		smiapp_set_compose_scaler(subdev, cfg, sel, crops, comp);
 
 	*comp = sel->r;
-	smiapp_propagate(subdev, cfg, sel->which,
-			 V4L2_SEL_TGT_COMPOSE);
+	smiapp_propagate(subdev, cfg, sel->which, V4L2_SEL_TGT_COMPOSE);
 
 	if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
 		return smiapp_update_mode(sensor);
@@ -2146,9 +2141,8 @@ static int smiapp_set_crop(struct v4l2_subdev *subdev,
 				->height;
 			src_size = &_r;
 		} else {
-			src_size =
-				v4l2_subdev_get_try_compose(
-					subdev, cfg, ssd->sink_pad);
+			src_size = v4l2_subdev_get_try_compose(
+				subdev, cfg, ssd->sink_pad);
 		}
 	}
 
@@ -2617,7 +2611,8 @@ static int smiapp_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 	for (i = 0; i < ssd->npads; i++) {
 		struct v4l2_mbus_framefmt *try_fmt =
 			v4l2_subdev_get_try_format(sd, fh->pad, i);
-		struct v4l2_rect *try_crop = v4l2_subdev_get_try_crop(sd, fh->pad, i);
+		struct v4l2_rect *try_crop =
+			v4l2_subdev_get_try_crop(sd, fh->pad, i);
 		struct v4l2_rect *try_comp;
 
 		smiapp_get_native_size(ssd, try_crop);
@@ -2856,8 +2851,7 @@ static int smiapp_probe(struct i2c_client *client,
 		return -EPROBE_DEFER;
 	}
 
-	rval = clk_set_rate(sensor->ext_clk,
-			    sensor->hwcfg->ext_clk);
+	rval = clk_set_rate(sensor->ext_clk, sensor->hwcfg->ext_clk);
 	if (rval < 0) {
 		dev_err(&client->dev,
 			"unable to set clock freq to %u\n",
@@ -2958,7 +2952,6 @@ static int smiapp_probe(struct i2c_client *client,
 		sensor->nvm = devm_kzalloc(&client->dev,
 				sensor->hwcfg->nvm_size, GFP_KERNEL);
 		if (sensor->nvm == NULL) {
-			dev_err(&client->dev, "nvm buf allocation failed\n");
 			rval = -ENOMEM;
 			goto out_cleanup;
 		}
-- 
2.1.4

Re: [PATCH v2 14/17] smiapp: Remove useless newlines and other small cleanups

[Sebastian Reichel]

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

Hi,

On Thu, Sep 15, 2016 at 02:22:28PM +0300, Sakari Ailus wrote:
> The code probably has been unindented at some point but rewrapping has not
> been done. Do it now.
> 
> Also remove a useless memory allocation failure message.

Reviewed-By: Sebastian Reichel <sre@kernel.org>

-- Sebastian

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

[PATCH v2 15/17] smiapp: Obtain correct media bus code for try format

[Sakari Ailus]

The media bus code obtained for try format may have been a code that the
sensor did not even support. Use a supported code with the current pixel
order.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 3548225..521afc0 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2602,8 +2602,6 @@ static int smiapp_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 {
 	struct smiapp_subdev *ssd = to_smiapp_subdev(sd);
 	struct smiapp_sensor *sensor = ssd->sensor;
-	u32 mbus_code =
-		smiapp_csi_data_formats[smiapp_pixel_order(sensor)].code;
 	unsigned int i;
 
 	mutex_lock(&sensor->mutex);
@@ -2619,7 +2617,7 @@ static int smiapp_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 
 		try_fmt->width = try_crop->width;
 		try_fmt->height = try_crop->height;
-		try_fmt->code = mbus_code;
+		try_fmt->code = sensor->internal_csi_format->code;
 		try_fmt->field = V4L2_FIELD_NONE;
 
 		if (ssd != sensor->pixel_array)
-- 
2.1.4

Re: [PATCH v2 15/17] smiapp: Obtain correct media bus code for try format

[Sebastian Reichel]

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

Hi,

On Thu, Sep 15, 2016 at 02:22:29PM +0300, Sakari Ailus wrote:
> The media bus code obtained for try format may have been a code that the
> sensor did not even support. Use a supported code with the current pixel
> order.

Reviewed-By: Sebastian Reichel <sre@kernel.org>

-- Sebastian

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

[PATCH v2 16/17] smiapp: Drop a debug print on frame size and bit depth

[Sakari Ailus]

The first time the sensor is powered on, the information is not yet
available.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 521afc0..61827fd 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -922,12 +922,6 @@ static int smiapp_update_mode(struct smiapp_sensor *sensor)
 	unsigned int binning_mode;
 	int rval;
 
-	dev_dbg(&client->dev, "frame size: %dx%d\n",
-		sensor->src->crop[SMIAPP_PAD_SRC].width,
-		sensor->src->crop[SMIAPP_PAD_SRC].height);
-	dev_dbg(&client->dev, "csi format width: %d\n",
-		sensor->csi_format->width);
-
 	/* Binning has to be set up here; it affects limits */
 	if (sensor->binning_horizontal == 1 &&
 	    sensor->binning_vertical == 1) {
-- 
2.1.4

Re: [PATCH v2 16/17] smiapp: Drop a debug print on frame size and bit depth

[Sebastian Reichel]

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

Hi,

On Thu, Sep 15, 2016 at 02:22:30PM +0300, Sakari Ailus wrote:
> The first time the sensor is powered on, the information is not yet
> available.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-By: Sebastian Reichel <sre@kernel.org>

-- Sebastian

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

[PATCH v2 17/17] smiapp-pll: Don't complain aloud about failing PLL calculation

[Sakari Ailus]

Don't complain about a failure to compute the pre_pll divisor. The
function is used to determine whether a particular combination of bits per
sample value and a link frequency can be used, in which case there are
lots of unnecessary driver messages. During normal operation the failure
generally does not happen. Use dev_dbg() instead.

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

diff --git a/drivers/media/i2c/smiapp-pll.c b/drivers/media/i2c/smiapp-pll.c
index e3348db..771db56 100644
--- a/drivers/media/i2c/smiapp-pll.c
+++ b/drivers/media/i2c/smiapp-pll.c
@@ -479,7 +479,8 @@ int smiapp_pll_calculate(struct device *dev,
 		return 0;
 	}
 
-	dev_info(dev, "unable to compute pre_pll divisor\n");
+	dev_dbg(dev, "unable to compute pre_pll divisor\n");
+
 	return rval;
 }
 EXPORT_SYMBOL_GPL(smiapp_pll_calculate);
-- 
2.1.4

Re: [PATCH v2 17/17] smiapp-pll: Don't complain aloud about failing PLL calculation

[Sebastian Reichel]

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

Hi,

On Thu, Sep 15, 2016 at 02:22:31PM +0300, Sakari Ailus wrote:
> Don't complain about a failure to compute the pre_pll divisor. The
> function is used to determine whether a particular combination of bits per
> sample value and a link frequency can be used, in which case there are
> lots of unnecessary driver messages. During normal operation the failure
> generally does not happen. Use dev_dbg() instead.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-By: Sebastian Reichel <sre@kernel.org>

-- Sebastian

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