[PATCH 0/6] Provide a serialisation mechanism for subdev ops

[PATCH 0/6] Provide a serialisation mechanism for subdev ops

[Sakari Ailus]

Hi folks,

While access to the V4L2 device nodes was conveniently serialised for
devices the drivers of which used the V4L2 framework, this was no the case
for sub-devices that also may be called from other drivers.

Instead acquire the lock to the v4l2_subdev_call macro.

This set adds the capability to the framework and makes smiapp use it.

This is a big change. I'm still not posting this as RFC as the feature is
entirely optional. Albeit I'd expect new drivers to use it in the future:
quite a bit of code was removed from the smiapp driver as a result (there
were other cleanups, too).

Generally drivers that handle multiple sub-devices need to take special
care when making nested calls (hopefully as few as possible, but starting
streaming comes to mind). We may need an unlocked variant of the macro as
well.

Sakari Ailus (6):
  v4l: subdev: Set sd->devnode before registering the subdev
  v4l: subdev: Provide a locking scheme for subdev operations
  smiapp: Error handling cleanups and fixes
  smiapp: Rely on V4L2 sub-device framework to do the locking
  smiapp: Remove the active field from sensor's struct
  smiapp: Avoid fall-through in switch

 drivers/media/i2c/smiapp/smiapp-core.c | 203 ++++++++-----------------
 drivers/media/i2c/smiapp/smiapp-regs.c |   3 -
 drivers/media/i2c/smiapp/smiapp.h      |   1 -
 drivers/media/v4l2-core/v4l2-device.c  |   3 +-
 include/media/v4l2-subdev.h            |  25 ++-
 5 files changed, 86 insertions(+), 149 deletions(-)

-- 
2.20.1

[PATCH 1/6] v4l: subdev: Set sd->devnode before registering the subdev

[Sakari Ailus]

The subdev's video device node was only assigned after registering the
device node in the system. While it is unlikely that a driver needed to
use this field in handling system calls to its file handle, there remains
a slim chance the devnode field remains NULL while the driver expects to
find a video node there.

Assign the devnode field before registering the device, and assign it back
to NULL if the registration failed.

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

diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index aa277f5bc862..8c79699b1be7 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -248,13 +248,14 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
 		vdev->fops = &v4l2_subdev_fops;
 		vdev->release = v4l2_device_release_subdev_node;
 		vdev->ctrl_handler = sd->ctrl_handler;
+		sd->devnode = vdev;
 		err = __video_register_device(vdev, VFL_TYPE_SUBDEV, -1, 1,
 					      sd->owner);
 		if (err < 0) {
+			sd->devnode = NULL;
 			kfree(vdev);
 			goto clean_up;
 		}
-		sd->devnode = vdev;
 #if defined(CONFIG_MEDIA_CONTROLLER)
 		sd->entity.info.dev.major = VIDEO_MAJOR;
 		sd->entity.info.dev.minor = vdev->minor;
-- 
2.20.1

[PATCH 2/6] v4l: subdev: Provide a locking scheme for subdev operations

[Sakari Ailus]

The V4L2 sub-device's operations are called both from other drivers as
well as through the IOCTL uAPI. Previously the sub-device drivers were
responsible for managing their own serialisation. This patch adds an
optional mutex the drivers may set, and it will be used to serialise
access to driver's data related to a device across the driver's ops.

Access to the driver's controls through the control framework works as
usual, i.e. using a different mutex.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/media/v4l2-subdev.h | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 71f1f2f0da53..dc6e11019df6 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -9,6 +9,7 @@
 #define _V4L2_SUBDEV_H
 
 #include <linux/types.h>
+#include <linux/mutex.h>
 #include <linux/v4l2-subdev.h>
 #include <media/media-entity.h>
 #include <media/v4l2-async.h>
@@ -828,6 +829,7 @@ struct v4l2_subdev_platform_data {
  * @host_priv: pointer to private data used by the device where the subdev
  *	is attached.
  * @devnode: subdev device node
+ * @lock: A mutex for serialising access to the subdev's operations. Optional.
  * @dev: pointer to the physical device, if any
  * @fwnode: The fwnode_handle of the subdev, usually the same as
  *	    either dev->of_node->fwnode or dev->fwnode (whichever is non-NULL).
@@ -862,6 +864,7 @@ struct v4l2_subdev {
 	void *dev_priv;
 	void *host_priv;
 	struct video_device *devnode;
+	struct mutex *lock;
 	struct device *dev;
 	struct fwnode_handle *fwnode;
 	struct list_head async_list;
@@ -1101,16 +1104,22 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
 	({								\
 		struct v4l2_subdev *__sd = (sd);			\
 		int __result;						\
-		if (!__sd)						\
+		if (!__sd) {						\
 			__result = -ENODEV;				\
-		else if (!(__sd->ops->o && __sd->ops->o->f))		\
+		} else if (!(__sd->ops->o && __sd->ops->o->f)) {	\
 			__result = -ENOIOCTLCMD;			\
-		else if (v4l2_subdev_call_wrappers.o &&			\
-			 v4l2_subdev_call_wrappers.o->f)		\
-			__result = v4l2_subdev_call_wrappers.o->f(	\
-							__sd, ##args);	\
-		else							\
-			__result = __sd->ops->o->f(__sd, ##args);	\
+		} else {						\
+			if (__sd->lock)					\
+				mutex_lock(__sd->lock);			\
+			if (v4l2_subdev_call_wrappers.o &&		\
+				 v4l2_subdev_call_wrappers.o->f)	\
+				__result = v4l2_subdev_call_wrappers.o->f( \
+					__sd, ##args);			\
+			else						\
+				__result = __sd->ops->o->f(__sd, ##args); \
+			if (__sd->lock)					\
+				mutex_unlock(__sd->lock);			\
+		}							\
 		__result;						\
 	})
 
-- 
2.20.1

[PATCH 3/6] smiapp: Error handling cleanups and fixes

[Sakari Ailus]

Probe error handling cleanups and fixes:

- Move mutex initialisation at a later time for easier error handling.

- Issue destroy_mutex on the sensor mutex on probe failure and driver
  remove,

- Remove smiapp_cleanup and add its contents in probe / remove on right
  locations.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 9adf8e034e7d..c9d0416f9b03 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2579,16 +2579,6 @@ static int smiapp_registered(struct v4l2_subdev *subdev)
 	return rval;
 }
 
-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 void smiapp_create_subdev(struct smiapp_sensor *sensor,
 				 struct smiapp_subdev *ssd, const char *name,
 				 unsigned short num_pads)
@@ -2862,7 +2852,6 @@ static int smiapp_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	sensor->hwcfg = hwcfg;
-	mutex_init(&sensor->mutex);
 	sensor->src = &sensor->ssds[sensor->ssds_used];
 
 	v4l2_i2c_subdev_init(&sensor->src->sd, client, &smiapp_ops);
@@ -2922,9 +2911,11 @@ static int smiapp_probe(struct i2c_client *client)
 	if (IS_ERR(sensor->xshutdown))
 		return PTR_ERR(sensor->xshutdown);
 
+	mutex_init(&sensor->mutex);
+
 	rval = smiapp_power_on(&client->dev);
 	if (rval < 0)
-		return rval;
+		goto out_mutex_destroy;
 
 	rval = smiapp_identify_module(sensor);
 	if (rval) {
@@ -3011,13 +3002,13 @@ static int smiapp_probe(struct i2c_client *client)
 				sensor->hwcfg->nvm_size, GFP_KERNEL);
 		if (sensor->nvm == NULL) {
 			rval = -ENOMEM;
-			goto out_cleanup;
+			goto out_remove_ident_attr;
 		}
 
 		if (device_create_file(&client->dev, &dev_attr_nvm) != 0) {
 			dev_err(&client->dev, "sysfs nvm entry failed\n");
 			rval = -EBUSY;
-			goto out_cleanup;
+			goto out_remove_ident_attr;
 		}
 	}
 
@@ -3067,22 +3058,22 @@ static int smiapp_probe(struct i2c_client *client)
 
 	rval = smiapp_init_controls(sensor);
 	if (rval < 0)
-		goto out_cleanup;
+		goto out_free_controls;
 
 	rval = smiapp_call_quirk(sensor, init);
 	if (rval)
-		goto out_cleanup;
+		goto out_free_controls;
 
 	rval = smiapp_get_mbus_formats(sensor);
 	if (rval) {
 		rval = -ENODEV;
-		goto out_cleanup;
+		goto out_free_controls;
 	}
 
 	rval = smiapp_init_late_controls(sensor);
 	if (rval) {
 		rval = -ENODEV;
-		goto out_cleanup;
+		goto out_free_controls;
 	}
 
 	mutex_lock(&sensor->mutex);
@@ -3090,7 +3081,7 @@ static int smiapp_probe(struct i2c_client *client)
 	mutex_unlock(&sensor->mutex);
 	if (rval) {
 		dev_err(&client->dev, "update mode failed\n");
-		goto out_cleanup;
+		goto out_free_controls;
 	}
 
 	sensor->streaming = false;
@@ -3117,12 +3108,19 @@ 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_remove_ident_attr:
+	device_remove_file(&client->dev, &dev_attr_ident);
+
+out_free_controls:
+	device_remove_file(&client->dev, &dev_attr_nvm);
+	smiapp_free_controls(sensor);
 
 out_power_off:
 	smiapp_power_off(&client->dev);
 
+out_mutex_destroy:
+	mutex_destroy(&sensor->mutex);
+
 	return rval;
 }
 
@@ -3143,7 +3141,13 @@ static int smiapp_remove(struct i2c_client *client)
 		v4l2_device_unregister_subdev(&sensor->ssds[i].sd);
 		media_entity_cleanup(&sensor->ssds[i].sd.entity);
 	}
-	smiapp_cleanup(sensor);
+
+	device_remove_file(&client->dev, &dev_attr_ident);
+	device_remove_file(&client->dev, &dev_attr_nvm);
+
+	smiapp_free_controls(sensor);
+
+	mutex_destroy(&sensor->mutex);
 
 	return 0;
 }
-- 
2.20.1

[PATCH 4/6] smiapp: Rely on V4L2 sub-device framework to do the locking

[Sakari Ailus]

Rely on the V4L2 sub-device framework to perform the locking for this
driver.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index c9d0416f9b03..79330ec41392 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -1228,8 +1228,6 @@ static int smiapp_power_on(struct device *dev)
 	sleep = SMIAPP_RESET_DELAY(sensor->hwcfg->ext_clk);
 	usleep_range(sleep, sleep);
 
-	mutex_lock(&sensor->mutex);
-
 	sensor->active = true;
 
 	/*
@@ -1330,12 +1328,9 @@ static int smiapp_power_on(struct device *dev)
 			goto out_cci_addr_fail;
 	}
 
-	mutex_unlock(&sensor->mutex);
-
 	return 0;
 
 out_cci_addr_fail:
-	mutex_unlock(&sensor->mutex);
 	gpiod_set_value(sensor->xshutdown, 0);
 	clk_disable_unprepare(sensor->ext_clk);
 
@@ -1353,8 +1348,6 @@ static int smiapp_power_off(struct device *dev)
 	struct smiapp_sensor *sensor =
 		container_of(ssd, struct smiapp_sensor, ssds[0]);
 
-	mutex_lock(&sensor->mutex);
-
 	/*
 	 * Currently power/clock to lens are enable/disabled separately
 	 * but they are essentially the same signals. So if the sensor is
@@ -1369,8 +1362,6 @@ static int smiapp_power_off(struct device *dev)
 
 	sensor->active = false;
 
-	mutex_unlock(&sensor->mutex);
-
 	gpiod_set_value(sensor->xshutdown, 0);
 	clk_disable_unprepare(sensor->ext_clk);
 	usleep_range(5000, 5000);
@@ -1389,28 +1380,26 @@ static int smiapp_start_streaming(struct smiapp_sensor *sensor)
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
 	int rval;
 
-	mutex_lock(&sensor->mutex);
-
 	rval = smiapp_write(sensor, SMIAPP_REG_U16_CSI_DATA_FORMAT,
 			    (sensor->csi_format->width << 8) |
 			    sensor->csi_format->compressed);
 	if (rval)
-		goto out;
+		return rval;
 
 	rval = smiapp_pll_configure(sensor);
 	if (rval)
-		goto out;
+		return rval;
 
 	/* Analog crop start coordinates */
 	rval = smiapp_write(sensor, SMIAPP_REG_U16_X_ADDR_START,
 			    sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].left);
 	if (rval < 0)
-		goto out;
+		return rval;
 
 	rval = smiapp_write(sensor, SMIAPP_REG_U16_Y_ADDR_START,
 			    sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].top);
 	if (rval < 0)
-		goto out;
+		return rval;
 
 	/* Analog crop end coordinates */
 	rval = smiapp_write(
@@ -1418,14 +1407,14 @@ static int smiapp_start_streaming(struct smiapp_sensor *sensor)
 		sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].left
 		+ sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].width - 1);
 	if (rval < 0)
-		goto out;
+		return rval;
 
 	rval = smiapp_write(
 		sensor, SMIAPP_REG_U16_Y_ADDR_END,
 		sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].top
 		+ sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].height - 1);
 	if (rval < 0)
-		goto out;
+		return rval;
 
 	/*
 	 * Output from pixel array, including blanking, is set using
@@ -1439,25 +1428,25 @@ static int smiapp_start_streaming(struct smiapp_sensor *sensor)
 			sensor, SMIAPP_REG_U16_DIGITAL_CROP_X_OFFSET,
 			sensor->scaler->crop[SMIAPP_PAD_SINK].left);
 		if (rval < 0)
-			goto out;
+			return rval;
 
 		rval = smiapp_write(
 			sensor, SMIAPP_REG_U16_DIGITAL_CROP_Y_OFFSET,
 			sensor->scaler->crop[SMIAPP_PAD_SINK].top);
 		if (rval < 0)
-			goto out;
+			return rval;
 
 		rval = smiapp_write(
 			sensor, SMIAPP_REG_U16_DIGITAL_CROP_IMAGE_WIDTH,
 			sensor->scaler->crop[SMIAPP_PAD_SINK].width);
 		if (rval < 0)
-			goto out;
+			return rval;
 
 		rval = smiapp_write(
 			sensor, SMIAPP_REG_U16_DIGITAL_CROP_IMAGE_HEIGHT,
 			sensor->scaler->crop[SMIAPP_PAD_SINK].height);
 		if (rval < 0)
-			goto out;
+			return rval;
 	}
 
 	/* Scaling */
@@ -1466,23 +1455,23 @@ static int smiapp_start_streaming(struct smiapp_sensor *sensor)
 		rval = smiapp_write(sensor, SMIAPP_REG_U16_SCALING_MODE,
 				    sensor->scaling_mode);
 		if (rval < 0)
-			goto out;
+			return rval;
 
 		rval = smiapp_write(sensor, SMIAPP_REG_U16_SCALE_M,
 				    sensor->scale_m);
 		if (rval < 0)
-			goto out;
+			return rval;
 	}
 
 	/* Output size from sensor */
 	rval = smiapp_write(sensor, SMIAPP_REG_U16_X_OUTPUT_SIZE,
 			    sensor->src->crop[SMIAPP_PAD_SRC].width);
 	if (rval < 0)
-		goto out;
+		return rval;
 	rval = smiapp_write(sensor, SMIAPP_REG_U16_Y_OUTPUT_SIZE,
 			    sensor->src->crop[SMIAPP_PAD_SRC].height);
 	if (rval < 0)
-		goto out;
+		return rval;
 
 	if ((sensor->limits[SMIAPP_LIMIT_FLASH_MODE_CAPABILITY] &
 	     (SMIAPP_FLASH_MODE_CAPABILITY_SINGLE_STROBE |
@@ -1491,21 +1480,18 @@ static int smiapp_start_streaming(struct smiapp_sensor *sensor)
 	    sensor->hwcfg->strobe_setup->trigger != 0) {
 		rval = smiapp_setup_flash_strobe(sensor);
 		if (rval)
-			goto out;
+			return rval;
 	}
 
 	rval = smiapp_call_quirk(sensor, pre_streamon);
 	if (rval) {
 		dev_err(&client->dev, "pre_streamon quirks failed\n");
-		goto out;
+		return rval;
 	}
 
 	rval = smiapp_write(sensor, SMIAPP_REG_U8_MODE_SELECT,
 			    SMIAPP_MODE_SELECT_STREAMING);
 
-out:
-	mutex_unlock(&sensor->mutex);
-
 	return rval;
 }
 
@@ -1514,18 +1500,15 @@ static int smiapp_stop_streaming(struct smiapp_sensor *sensor)
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
 	int rval;
 
-	mutex_lock(&sensor->mutex);
 	rval = smiapp_write(sensor, SMIAPP_REG_U8_MODE_SELECT,
 			    SMIAPP_MODE_SELECT_SOFTWARE_STANDBY);
 	if (rval)
-		goto out;
+		return rval;
 
 	rval = smiapp_call_quirk(sensor, post_streamoff);
 	if (rval)
 		dev_err(&client->dev, "post_streamoff quirks failed\n");
 
-out:
-	mutex_unlock(&sensor->mutex);
 	return rval;
 }
 
@@ -1574,20 +1557,17 @@ static int smiapp_enum_mbus_code(struct v4l2_subdev *subdev,
 	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
 	unsigned int i;
 	int idx = -1;
-	int rval = -EINVAL;
-
-	mutex_lock(&sensor->mutex);
+	int rval = 0;
 
 	dev_err(&client->dev, "subdev %s, pad %d, index %d\n",
 		subdev->name, code->pad, code->index);
 
 	if (subdev != &sensor->src->sd || code->pad != SMIAPP_PAD_SRC) {
 		if (code->index)
-			goto out;
+			return -EINVAL;
 
 		code->code = sensor->internal_csi_format->code;
-		rval = 0;
-		goto out;
+		return 0;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(smiapp_csi_data_formats); i++) {
@@ -1598,14 +1578,10 @@ static int smiapp_enum_mbus_code(struct v4l2_subdev *subdev,
 			code->code = smiapp_csi_data_formats[i].code;
 			dev_err(&client->dev, "found index %d, i %d, code %x\n",
 				code->index, i, code->code);
-			rval = 0;
-			break;
+			return 0;
 		}
 	}
 
-out:
-	mutex_unlock(&sensor->mutex);
-
 	return rval;
 }
 
@@ -1620,9 +1596,9 @@ static u32 __smiapp_get_mbus_code(struct v4l2_subdev *subdev,
 		return sensor->internal_csi_format->code;
 }
 
-static int __smiapp_get_format(struct v4l2_subdev *subdev,
-			       struct v4l2_subdev_pad_config *cfg,
-			       struct v4l2_subdev_format *fmt)
+static int smiapp_get_format(struct v4l2_subdev *subdev,
+			     struct v4l2_subdev_pad_config *cfg,
+			     struct v4l2_subdev_format *fmt)
 {
 	struct smiapp_subdev *ssd = to_smiapp_subdev(subdev);
 
@@ -1646,20 +1622,6 @@ static int __smiapp_get_format(struct v4l2_subdev *subdev,
 	return 0;
 }
 
-static int smiapp_get_format(struct v4l2_subdev *subdev,
-			     struct v4l2_subdev_pad_config *cfg,
-			     struct v4l2_subdev_format *fmt)
-{
-	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
-	int rval;
-
-	mutex_lock(&sensor->mutex);
-	rval = __smiapp_get_format(subdev, cfg, fmt);
-	mutex_unlock(&sensor->mutex);
-
-	return rval;
-}
-
 static void smiapp_get_crop_compose(struct v4l2_subdev *subdev,
 				    struct v4l2_subdev_pad_config *cfg,
 				    struct v4l2_rect **crops,
@@ -1751,7 +1713,7 @@ static int smiapp_set_format_source(struct v4l2_subdev *subdev,
 	unsigned int i;
 	int rval;
 
-	rval = __smiapp_get_format(subdev, cfg, fmt);
+	rval = smiapp_get_format(subdev, cfg, fmt);
 	if (rval)
 		return rval;
 
@@ -1800,17 +1762,8 @@ static int smiapp_set_format(struct v4l2_subdev *subdev,
 	struct smiapp_subdev *ssd = to_smiapp_subdev(subdev);
 	struct v4l2_rect *crops[SMIAPP_PADS];
 
-	mutex_lock(&sensor->mutex);
-
-	if (fmt->pad == ssd->source_pad) {
-		int rval;
-
-		rval = smiapp_set_format_source(subdev, cfg, fmt);
-
-		mutex_unlock(&sensor->mutex);
-
-		return rval;
-	}
+	if (fmt->pad == ssd->source_pad)
+		return smiapp_set_format_source(subdev, cfg, fmt);
 
 	/* Sink pad. Width and height are changeable here. */
 	fmt->format.code = __smiapp_get_mbus_code(subdev, fmt->pad);
@@ -1838,8 +1791,6 @@ static int smiapp_set_format(struct v4l2_subdev *subdev,
 	smiapp_propagate(subdev, cfg, fmt->which,
 			 V4L2_SEL_TGT_CROP);
 
-	mutex_unlock(&sensor->mutex);
-
 	return 0;
 }
 
@@ -2181,9 +2132,9 @@ static void smiapp_get_native_size(struct smiapp_subdev *ssd,
 	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)
+static int smiapp_get_selection(struct v4l2_subdev *subdev,
+				struct v4l2_subdev_pad_config *cfg,
+				struct v4l2_subdev_selection *sel)
 {
 	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
 	struct smiapp_subdev *ssd = to_smiapp_subdev(subdev);
@@ -2231,19 +2182,6 @@ static int __smiapp_get_selection(struct v4l2_subdev *subdev,
 	return 0;
 }
 
-static int smiapp_get_selection(struct v4l2_subdev *subdev,
-				struct v4l2_subdev_pad_config *cfg,
-				struct v4l2_subdev_selection *sel)
-{
-	struct smiapp_sensor *sensor = to_smiapp_sensor(subdev);
-	int rval;
-
-	mutex_lock(&sensor->mutex);
-	rval = __smiapp_get_selection(subdev, cfg, sel);
-	mutex_unlock(&sensor->mutex);
-
-	return rval;
-}
 static int smiapp_set_selection(struct v4l2_subdev *subdev,
 				struct v4l2_subdev_pad_config *cfg,
 				struct v4l2_subdev_selection *sel)
@@ -2255,8 +2193,6 @@ static int smiapp_set_selection(struct v4l2_subdev *subdev,
 	if (ret)
 		return ret;
 
-	mutex_lock(&sensor->mutex);
-
 	sel->r.left = max(0, sel->r.left & ~1);
 	sel->r.top = max(0, sel->r.top & ~1);
 	sel->r.width = SMIAPP_ALIGN_DIM(sel->r.width, sel->flags);
@@ -2271,17 +2207,14 @@ static int smiapp_set_selection(struct v4l2_subdev *subdev,
 
 	switch (sel->target) {
 	case V4L2_SEL_TGT_CROP:
-		ret = smiapp_set_crop(subdev, cfg, sel);
+		return smiapp_set_crop(subdev, cfg, sel);
 		break;
 	case V4L2_SEL_TGT_COMPOSE:
-		ret = smiapp_set_compose(subdev, cfg, sel);
+		return smiapp_set_compose(subdev, cfg, sel);
 		break;
 	default:
-		ret = -EINVAL;
+		return -EINVAL;
 	}
-
-	mutex_unlock(&sensor->mutex);
-	return ret;
 }
 
 static int smiapp_get_skip_frames(struct v4l2_subdev *subdev, u32 *frames)
@@ -2627,8 +2560,6 @@ static int smiapp_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 	struct smiapp_sensor *sensor = ssd->sensor;
 	unsigned int i;
 
-	mutex_lock(&sensor->mutex);
-
 	for (i = 0; i < ssd->npads; i++) {
 		struct v4l2_mbus_framefmt *try_fmt =
 			v4l2_subdev_get_try_format(sd, fh->pad, i);
@@ -2650,8 +2581,6 @@ static int smiapp_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 		*try_comp = *try_crop;
 	}
 
-	mutex_unlock(&sensor->mutex);
-
 	return 0;
 }
 
@@ -2913,6 +2842,9 @@ static int smiapp_probe(struct i2c_client *client)
 
 	mutex_init(&sensor->mutex);
 
+	for (i = 0; i < SMIAPP_SUBDEVS; i++)
+		sensor->ssds[i].sd.lock = &sensor->mutex;
+
 	rval = smiapp_power_on(&client->dev);
 	if (rval < 0)
 		goto out_mutex_destroy;
-- 
2.20.1

[PATCH 5/6] smiapp: Remove the active field from sensor's struct

[Sakari Ailus]

The active field was used to track whether the sensor was powered when
settings its controls. This is no longer needed as runtime PM is used to
determine this instead.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 79330ec41392..76d7d204ec17 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -1228,8 +1228,6 @@ static int smiapp_power_on(struct device *dev)
 	sleep = SMIAPP_RESET_DELAY(sensor->hwcfg->ext_clk);
 	usleep_range(sleep, sleep);
 
-	sensor->active = true;
-
 	/*
 	 * Failures to respond to the address change command have been noticed.
 	 * Those failures seem to be caused by the sensor requiring a longer
@@ -1360,8 +1358,6 @@ static int smiapp_power_off(struct device *dev)
 			     SMIAPP_REG_U8_SOFTWARE_RESET,
 			     SMIAPP_SOFTWARE_RESET);
 
-	sensor->active = false;
-
 	gpiod_set_value(sensor->xshutdown, 0);
 	clk_disable_unprepare(sensor->ext_clk);
 	usleep_range(5000, 5000);
diff --git a/drivers/media/i2c/smiapp/smiapp-regs.c b/drivers/media/i2c/smiapp/smiapp-regs.c
index 0470e47c2f7a..ce8c1d47fbf0 100644
--- a/drivers/media/i2c/smiapp/smiapp-regs.c
+++ b/drivers/media/i2c/smiapp/smiapp-regs.c
@@ -223,9 +223,6 @@ int smiapp_write_no_quirk(struct smiapp_sensor *sensor, u32 reg, u32 val)
 	     len != SMIAPP_REG_32BIT) || flags)
 		return -EINVAL;
 
-	if (!sensor->active)
-		return 0;
-
 	msg.addr = client->addr;
 	msg.flags = 0; /* Write */
 	msg.len = 2 + len;
diff --git a/drivers/media/i2c/smiapp/smiapp.h b/drivers/media/i2c/smiapp/smiapp.h
index ecf8a17dbe37..75e151e78617 100644
--- a/drivers/media/i2c/smiapp/smiapp.h
+++ b/drivers/media/i2c/smiapp/smiapp.h
@@ -198,7 +198,6 @@ struct smiapp_sensor {
 
 	u8 hvflip_inv_mask; /* H/VFLIP inversion due to sensor orientation */
 	u8 frame_skip;
-	bool active; /* is the sensor powered on? */
 	u16 embedded_start; /* embedded data start line */
 	u16 embedded_end;
 	u16 image_start; /* image data start line */
-- 
2.20.1

[PATCH 6/6] smiapp: Avoid fall-through in switch

[Sakari Ailus]

Remove switch fall-through cases in the driver.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 76d7d204ec17..61de8cdccc4b 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -1674,13 +1674,12 @@ static void smiapp_propagate(struct v4l2_subdev *subdev,
 				sensor->binning_vertical = 1;
 			}
 		}
-		/* Fall through */
-	case V4L2_SEL_TGT_COMPOSE:
-		*crops[SMIAPP_PAD_SRC] = *comp;
 		break;
 	default:
-		BUG();
+		WARN_ON(1);
+		return;
 	}
+	*crops[SMIAPP_PAD_SRC] = *comp;
 }
 
 static const struct smiapp_csi_data_format
@@ -2062,7 +2061,7 @@ static int __smiapp_sel_supported(struct v4l2_subdev *subdev,
 		    && sensor->limits[SMIAPP_LIMIT_SCALING_CAPABILITY]
 		    != SMIAPP_SCALING_CAPABILITY_NONE)
 			return 0;
-		/* Fall through */
+		return -EINVAL;
 	default:
 		return -EINVAL;
 	}
@@ -2716,7 +2715,7 @@ static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
 		case 180:
 			hwcfg->module_board_orient =
 				SMIAPP_MODULE_BOARD_ORIENT_180;
-			/* Fall through */
+			break;
 		case 0:
 			break;
 		default:
-- 
2.20.1

Re: [PATCH 1/6] v4l: subdev: Set sd->devnode before registering the subdev

[Hans Verkuil]

On 8/19/19 2:47 PM, Sakari Ailus wrote:
> The subdev's video device node was only assigned after registering the
> device node in the system. While it is unlikely that a driver needed to
> use this field in handling system calls to its file handle, there remains
> a slim chance the devnode field remains NULL while the driver expects to
> find a video node there.
> 
> Assign the devnode field before registering the device, and assign it back
> to NULL if the registration failed.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

> ---
>  drivers/media/v4l2-core/v4l2-device.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
> index aa277f5bc862..8c79699b1be7 100644
> --- a/drivers/media/v4l2-core/v4l2-device.c
> +++ b/drivers/media/v4l2-core/v4l2-device.c
> @@ -248,13 +248,14 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
>  		vdev->fops = &v4l2_subdev_fops;
>  		vdev->release = v4l2_device_release_subdev_node;
>  		vdev->ctrl_handler = sd->ctrl_handler;
> +		sd->devnode = vdev;
>  		err = __video_register_device(vdev, VFL_TYPE_SUBDEV, -1, 1,
>  					      sd->owner);
>  		if (err < 0) {
> +			sd->devnode = NULL;
>  			kfree(vdev);
>  			goto clean_up;
>  		}
> -		sd->devnode = vdev;
>  #if defined(CONFIG_MEDIA_CONTROLLER)
>  		sd->entity.info.dev.major = VIDEO_MAJOR;
>  		sd->entity.info.dev.minor = vdev->minor;
> 

Re: [PATCH 2/6] v4l: subdev: Provide a locking scheme for subdev operations

[Hans Verkuil]

On 8/19/19 2:47 PM, Sakari Ailus wrote:
> The V4L2 sub-device's operations are called both from other drivers as
> well as through the IOCTL uAPI. Previously the sub-device drivers were
> responsible for managing their own serialisation. This patch adds an
> optional mutex the drivers may set, and it will be used to serialise
> access to driver's data related to a device across the driver's ops.
> 
> Access to the driver's controls through the control framework works as
> usual, i.e. using a different mutex.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/media/v4l2-subdev.h | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 71f1f2f0da53..dc6e11019df6 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -9,6 +9,7 @@
>  #define _V4L2_SUBDEV_H
>  
>  #include <linux/types.h>
> +#include <linux/mutex.h>
>  #include <linux/v4l2-subdev.h>
>  #include <media/media-entity.h>
>  #include <media/v4l2-async.h>
> @@ -828,6 +829,7 @@ struct v4l2_subdev_platform_data {
>   * @host_priv: pointer to private data used by the device where the subdev
>   *	is attached.
>   * @devnode: subdev device node
> + * @lock: A mutex for serialising access to the subdev's operations. Optional.

A pointer to a mutex...

>   * @dev: pointer to the physical device, if any
>   * @fwnode: The fwnode_handle of the subdev, usually the same as
>   *	    either dev->of_node->fwnode or dev->fwnode (whichever is non-NULL).
> @@ -862,6 +864,7 @@ struct v4l2_subdev {
>  	void *dev_priv;
>  	void *host_priv;
>  	struct video_device *devnode;
> +	struct mutex *lock;
>  	struct device *dev;
>  	struct fwnode_handle *fwnode;
>  	struct list_head async_list;
> @@ -1101,16 +1104,22 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
>  	({								\
>  		struct v4l2_subdev *__sd = (sd);			\
>  		int __result;						\
> -		if (!__sd)						\
> +		if (!__sd) {						\
>  			__result = -ENODEV;				\
> -		else if (!(__sd->ops->o && __sd->ops->o->f))		\
> +		} else if (!(__sd->ops->o && __sd->ops->o->f)) {	\
>  			__result = -ENOIOCTLCMD;			\
> -		else if (v4l2_subdev_call_wrappers.o &&			\
> -			 v4l2_subdev_call_wrappers.o->f)		\
> -			__result = v4l2_subdev_call_wrappers.o->f(	\
> -							__sd, ##args);	\
> -		else							\
> -			__result = __sd->ops->o->f(__sd, ##args);	\
> +		} else {						\
> +			if (__sd->lock)					\
> +				mutex_lock(__sd->lock);			\
> +			if (v4l2_subdev_call_wrappers.o &&		\
> +				 v4l2_subdev_call_wrappers.o->f)	\
> +				__result = v4l2_subdev_call_wrappers.o->f( \
> +					__sd, ##args);			\
> +			else						\
> +				__result = __sd->ops->o->f(__sd, ##args); \
> +			if (__sd->lock)					\
> +				mutex_unlock(__sd->lock);			\
> +		}							\
>  		__result;						\
>  	})
>  
> 

I'm not sure this is the right place to lock. Locking is only needed if the
subdev device can be called directly from userspace. So I would put the
locking in subdev_do_ioctl() and use mutex_lock_interruptible.

If there are subdev ops that in this scenario (i.e. userspace is responsible
for configuring the subdev) are still called from another driver, then I would
create a v4l2_subdev_call_lock() function that takes the lock.

Adding a lock in the v4l2_subdev_call macro feels too invasive.

Regards,

	Hans

Re: [PATCH 6/6] smiapp: Avoid fall-through in switch

[Hans Verkuil]

On 8/19/19 2:47 PM, Sakari Ailus wrote:
> Remove switch fall-through cases in the driver.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/i2c/smiapp/smiapp-core.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
> index 76d7d204ec17..61de8cdccc4b 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -1674,13 +1674,12 @@ static void smiapp_propagate(struct v4l2_subdev *subdev,
>  				sensor->binning_vertical = 1;
>  			}
>  		}
> -		/* Fall through */
> -	case V4L2_SEL_TGT_COMPOSE:

This doesn't look right: for this target you now enter the default case.

You probably want to do:

		break;
	case V4L2_SEL_TGT_COMPOSE:
		break;

Regards,

	Hans

> -		*crops[SMIAPP_PAD_SRC] = *comp;
>  		break;
>  	default:
> -		BUG();
> +		WARN_ON(1);
> +		return;
>  	}
> +	*crops[SMIAPP_PAD_SRC] = *comp;
>  }
>  
>  static const struct smiapp_csi_data_format
> @@ -2062,7 +2061,7 @@ static int __smiapp_sel_supported(struct v4l2_subdev *subdev,
>  		    && sensor->limits[SMIAPP_LIMIT_SCALING_CAPABILITY]
>  		    != SMIAPP_SCALING_CAPABILITY_NONE)
>  			return 0;
> -		/* Fall through */
> +		return -EINVAL;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -2716,7 +2715,7 @@ static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
>  		case 180:
>  			hwcfg->module_board_orient =
>  				SMIAPP_MODULE_BOARD_ORIENT_180;
> -			/* Fall through */
> +			break;
>  		case 0:
>  			break;
>  		default:
> 

Re: [PATCH 2/6] v4l: subdev: Provide a locking scheme for subdev operations

[Sakari Ailus]

Hi Hans,

On Thu, Sep 12, 2019 at 03:11:27PM +0200, Hans Verkuil wrote:
> On 8/19/19 2:47 PM, Sakari Ailus wrote:
> > The V4L2 sub-device's operations are called both from other drivers as
> > well as through the IOCTL uAPI. Previously the sub-device drivers were
> > responsible for managing their own serialisation. This patch adds an
> > optional mutex the drivers may set, and it will be used to serialise
> > access to driver's data related to a device across the driver's ops.
> > 
> > Access to the driver's controls through the control framework works as
> > usual, i.e. using a different mutex.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  include/media/v4l2-subdev.h | 25 +++++++++++++++++--------
> >  1 file changed, 17 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 71f1f2f0da53..dc6e11019df6 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -9,6 +9,7 @@
> >  #define _V4L2_SUBDEV_H
> >  
> >  #include <linux/types.h>
> > +#include <linux/mutex.h>
> >  #include <linux/v4l2-subdev.h>
> >  #include <media/media-entity.h>
> >  #include <media/v4l2-async.h>
> > @@ -828,6 +829,7 @@ struct v4l2_subdev_platform_data {
> >   * @host_priv: pointer to private data used by the device where the subdev
> >   *	is attached.
> >   * @devnode: subdev device node
> > + * @lock: A mutex for serialising access to the subdev's operations. Optional.
> 
> A pointer to a mutex...

Yes.

> 
> >   * @dev: pointer to the physical device, if any
> >   * @fwnode: The fwnode_handle of the subdev, usually the same as
> >   *	    either dev->of_node->fwnode or dev->fwnode (whichever is non-NULL).
> > @@ -862,6 +864,7 @@ struct v4l2_subdev {
> >  	void *dev_priv;
> >  	void *host_priv;
> >  	struct video_device *devnode;
> > +	struct mutex *lock;
> >  	struct device *dev;
> >  	struct fwnode_handle *fwnode;
> >  	struct list_head async_list;
> > @@ -1101,16 +1104,22 @@ extern const struct v4l2_subdev_ops v4l2_subdev_call_wrappers;
> >  	({								\
> >  		struct v4l2_subdev *__sd = (sd);			\
> >  		int __result;						\
> > -		if (!__sd)						\
> > +		if (!__sd) {						\
> >  			__result = -ENODEV;				\
> > -		else if (!(__sd->ops->o && __sd->ops->o->f))		\
> > +		} else if (!(__sd->ops->o && __sd->ops->o->f)) {	\
> >  			__result = -ENOIOCTLCMD;			\
> > -		else if (v4l2_subdev_call_wrappers.o &&			\
> > -			 v4l2_subdev_call_wrappers.o->f)		\
> > -			__result = v4l2_subdev_call_wrappers.o->f(	\
> > -							__sd, ##args);	\
> > -		else							\
> > -			__result = __sd->ops->o->f(__sd, ##args);	\
> > +		} else {						\
> > +			if (__sd->lock)					\
> > +				mutex_lock(__sd->lock);			\
> > +			if (v4l2_subdev_call_wrappers.o &&		\
> > +				 v4l2_subdev_call_wrappers.o->f)	\
> > +				__result = v4l2_subdev_call_wrappers.o->f( \
> > +					__sd, ##args);			\
> > +			else						\
> > +				__result = __sd->ops->o->f(__sd, ##args); \
> > +			if (__sd->lock)					\
> > +				mutex_unlock(__sd->lock);			\
> > +		}							\
> >  		__result;						\
> >  	})
> >  
> > 
> 
> I'm not sure this is the right place to lock. Locking is only needed if the
> subdev device can be called directly from userspace. So I would put the
> locking in subdev_do_ioctl() and use mutex_lock_interruptible.
> 
> If there are subdev ops that in this scenario (i.e. userspace is responsible
> for configuring the subdev) are still called from another driver, then I would
> create a v4l2_subdev_call_lock() function that takes the lock.
> 
> Adding a lock in the v4l2_subdev_call macro feels too invasive.

This is still the very purpose of the patch: provide drivers with a way to
have their operations serialised using a mutex. As the v4l2_subdev_call()
is the macro used to call sub-devices' operations, there's no other single
point to acquire (and release) the mutex.

If another name is chosen, then all (or almost all) current users would
need to switch to use the new macro in order for the change to be
effective.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

[PATCH v2 6/6] smiapp: Avoid fall-through in switch

[Sakari Ailus]

Remove switch fall-through cases in the driver.

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

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 76d7d204ec17..c6202f3a4015 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -1674,13 +1674,14 @@ static void smiapp_propagate(struct v4l2_subdev *subdev,
 				sensor->binning_vertical = 1;
 			}
 		}
-		/* Fall through */
+		break;
 	case V4L2_SEL_TGT_COMPOSE:
-		*crops[SMIAPP_PAD_SRC] = *comp;
 		break;
 	default:
-		BUG();
+		WARN_ON(1);
+		return;
 	}
+	*crops[SMIAPP_PAD_SRC] = *comp;
 }
 
 static const struct smiapp_csi_data_format
@@ -2062,7 +2063,7 @@ static int __smiapp_sel_supported(struct v4l2_subdev *subdev,
 		    && sensor->limits[SMIAPP_LIMIT_SCALING_CAPABILITY]
 		    != SMIAPP_SCALING_CAPABILITY_NONE)
 			return 0;
-		/* Fall through */
+		return -EINVAL;
 	default:
 		return -EINVAL;
 	}
@@ -2716,7 +2717,7 @@ static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
 		case 180:
 			hwcfg->module_board_orient =
 				SMIAPP_MODULE_BOARD_ORIENT_180;
-			/* Fall through */
+			break;
 		case 0:
 			break;
 		default:
-- 
2.20.1

Re: [PATCH 6/6] smiapp: Avoid fall-through in switch

[Sakari Ailus]

On Thu, Sep 12, 2019 at 03:17:54PM +0200, Hans Verkuil wrote:
> On 8/19/19 2:47 PM, Sakari Ailus wrote:
> > Remove switch fall-through cases in the driver.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/i2c/smiapp/smiapp-core.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
> > index 76d7d204ec17..61de8cdccc4b 100644
> > --- a/drivers/media/i2c/smiapp/smiapp-core.c
> > +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> > @@ -1674,13 +1674,12 @@ static void smiapp_propagate(struct v4l2_subdev *subdev,
> >  				sensor->binning_vertical = 1;
> >  			}
> >  		}
> > -		/* Fall through */
> > -	case V4L2_SEL_TGT_COMPOSE:
> 
> This doesn't look right: for this target you now enter the default case.
> 
> You probably want to do:
> 
> 		break;
> 	case V4L2_SEL_TGT_COMPOSE:
> 		break;

Yes; thanks. I've just sent v2.

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

Re: [PATCH v2 6/6] smiapp: Avoid fall-through in switch

[Hans Verkuil]

On 9/13/19 8:47 AM, Sakari Ailus wrote:
> Remove switch fall-through cases in the driver.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

> ---
>  drivers/media/i2c/smiapp/smiapp-core.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
> index 76d7d204ec17..c6202f3a4015 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -1674,13 +1674,14 @@ static void smiapp_propagate(struct v4l2_subdev *subdev,
>  				sensor->binning_vertical = 1;
>  			}
>  		}
> -		/* Fall through */
> +		break;
>  	case V4L2_SEL_TGT_COMPOSE:
> -		*crops[SMIAPP_PAD_SRC] = *comp;
>  		break;
>  	default:
> -		BUG();
> +		WARN_ON(1);
> +		return;
>  	}
> +	*crops[SMIAPP_PAD_SRC] = *comp;
>  }
>  
>  static const struct smiapp_csi_data_format
> @@ -2062,7 +2063,7 @@ static int __smiapp_sel_supported(struct v4l2_subdev *subdev,
>  		    && sensor->limits[SMIAPP_LIMIT_SCALING_CAPABILITY]
>  		    != SMIAPP_SCALING_CAPABILITY_NONE)
>  			return 0;
> -		/* Fall through */
> +		return -EINVAL;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -2716,7 +2717,7 @@ static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
>  		case 180:
>  			hwcfg->module_board_orient =
>  				SMIAPP_MODULE_BOARD_ORIENT_180;
> -			/* Fall through */
> +			break;
>  		case 0:
>  			break;
>  		default:
>