[PATCH v1 01/10] staging: atomisp: Don't leak GPIO resources if clk_get() failed

[PATCH v1 01/10] staging: atomisp: Don't leak GPIO resources if clk_get() failed

[Andy Shevchenko]

In case devm_clk_get() call fails the previously requested GPIOs are
left requested.

Fix this by moving GPIO request code after devm_clk_get() call.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 .../staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c  | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
index bf9f34b7ad72..a5d0dd88a8bc 100644
--- a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
@@ -322,8 +322,6 @@ static struct gmin_subdev *gmin_subdev_add(struct v4l2_subdev *subdev)
 							VLV2_CLK_PLL_19P2MHZ);
 	gmin_subdevs[i].csi_port = gmin_get_var_int(dev, "CsiPort", 0);
 	gmin_subdevs[i].csi_lanes = gmin_get_var_int(dev, "CsiLanes", 1);
-	gmin_subdevs[i].gpio0 = gpiod_get_index(dev, NULL, 0, GPIOD_OUT_LOW);
-	gmin_subdevs[i].gpio1 = gpiod_get_index(dev, NULL, 1, GPIOD_OUT_LOW);
 
 	/* get PMC clock with clock framework */
 	snprintf(gmin_pmc_clk_name,
@@ -356,9 +354,11 @@ static struct gmin_subdev *gmin_subdev_add(struct v4l2_subdev *subdev)
 	if (!ret)
 		clk_disable_unprepare(gmin_subdevs[i].pmc_clk);
 
+	gmin_subdevs[i].gpio0 = gpiod_get_index(dev, NULL, 0, GPIOD_OUT_LOW);
 	if (IS_ERR(gmin_subdevs[i].gpio0))
 		gmin_subdevs[i].gpio0 = NULL;
 
+	gmin_subdevs[i].gpio1 = gpiod_get_index(dev, NULL, 1, GPIOD_OUT_LOW);
 	if (IS_ERR(gmin_subdevs[i].gpio1))
 		gmin_subdevs[i].gpio1 = NULL;
 
-- 
2.15.1

[PATCH v1 02/10] staging: atomisp: Remove duplicate NULL-check

[Andy Shevchenko]

GPIO framework checks for NULL pointer when gpiod_set_value() is called.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 .../staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c  | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
index a5d0dd88a8bc..8fb5147531a5 100644
--- a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
@@ -394,7 +394,7 @@ static int gmin_gpio0_ctrl(struct v4l2_subdev *subdev, int on)
 {
 	struct gmin_subdev *gs = find_gmin_subdev(subdev);
 
-	if (gs && gs->gpio0) {
+	if (gs) {
 		gpiod_set_value(gs->gpio0, on);
 		return 0;
 	}
@@ -405,7 +405,7 @@ static int gmin_gpio1_ctrl(struct v4l2_subdev *subdev, int on)
 {
 	struct gmin_subdev *gs = find_gmin_subdev(subdev);
 
-	if (gs && gs->gpio1) {
+	if (gs) {
 		gpiod_set_value(gs->gpio1, on);
 		return 0;
 	}
-- 
2.15.1

[PATCH v1 03/10] staging: atomisp: lm3554: Fix control values

[Andy Shevchenko]

Driver fails to initialize due to insane settings in the
control init array.

Fix this by moving to sanity.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/staging/media/atomisp/i2c/atomisp-lm3554.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
index 4fd9f538ac95..974b6ff50c7a 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
@@ -562,10 +562,10 @@ static const struct v4l2_ctrl_config lm3554_controls[] = {
 	{
 	 .ops = &ctrl_ops,
 	 .id = V4L2_CID_FLASH_STATUS,
-	 .type = V4L2_CTRL_TYPE_BOOLEAN,
+	 .type = V4L2_CTRL_TYPE_INTEGER,
 	 .name = "Flash Status",
-	 .min = 0,
-	 .max = 100,
+	 .min = ATOMISP_FLASH_STATUS_OK,
+	 .max = ATOMISP_FLASH_STATUS_TIMEOUT,
 	 .step = 1,
 	 .def = ATOMISP_FLASH_STATUS_OK,
 	 .flags = 0,
@@ -574,10 +574,10 @@ static const struct v4l2_ctrl_config lm3554_controls[] = {
 	{
 	 .ops = &ctrl_ops,
 	 .id = V4L2_CID_FLASH_STATUS_REGISTER,
-	 .type = V4L2_CTRL_TYPE_BOOLEAN,
+	 .type = V4L2_CTRL_TYPE_INTEGER,
 	 .name = "Flash Status Register",
 	 .min = 0,
-	 .max = 100,
+	 .max = 255,
 	 .step = 1,
 	 .def = 0,
 	 .flags = 0,
-- 
2.15.1

[PATCH v1 04/10] staging: atomisp: Disable custom format for now

[Andy Shevchenko]

Custom video format 'M101' is not supported in upstream and as a result
user will get ugly warning:

 Unknown pixelformat 0x3130314d
 ------------[ cut here ]------------
 WARNING: CPU: 3 PID: 1574 at drivers/media/v4l2-core/v4l2-ioctl.c:1291 v4l_enum_fmt+0xcf1/0x13a0 [videodev]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/staging/media/atomisp/include/linux/atomisp.h       | 2 ++
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c  | 5 ++++-
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c | 2 ++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/include/linux/atomisp.h b/drivers/staging/media/atomisp/include/linux/atomisp.h
index 15fa5679bae7..ebe193ba3871 100644
--- a/drivers/staging/media/atomisp/include/linux/atomisp.h
+++ b/drivers/staging/media/atomisp/include/linux/atomisp.h
@@ -68,7 +68,9 @@
 #define V4L2_MBUS_FMT_CUSTOM_RGB32	0x800a
 
 /* Custom media bus format for M10MO RAW capture */
+#if 0
 #define V4L2_MBUS_FMT_CUSTOM_M10MO_RAW	0x800b
+#endif
 
 /* Configuration used by Bayer noise reduction and YCC noise reduction */
 struct atomisp_nr_config {
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c
index 339b5d31e1f1..5c84dd63778e 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c
@@ -501,7 +501,9 @@ const struct atomisp_format_bridge atomisp_output_fmts[] = {
 		.mbus_code = MEDIA_BUS_FMT_JPEG_1X8,
 		.sh_fmt = CSS_FRAME_FORMAT_BINARY_8,
 		.description = "JPEG"
-	}, {
+	},
+#if 0
+	{
 	/* This is a custom format being used by M10MO to send the RAW data */
 		.pixelformat = V4L2_PIX_FMT_CUSTOM_M10MO_RAW,
 		.depth = 8,
@@ -509,6 +511,7 @@ const struct atomisp_format_bridge atomisp_output_fmts[] = {
 		.sh_fmt = CSS_FRAME_FORMAT_BINARY_8,
 		.description = "Custom RAW for M10MO"
 	},
+#endif
 };
 
 const struct atomisp_format_bridge *atomisp_get_format_bridge(
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
index 70b53988553c..f3e18d627b0a 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
@@ -48,7 +48,9 @@ const struct atomisp_in_fmt_conv atomisp_in_fmt_conv[] = {
 	{ V4L2_MBUS_FMT_CUSTOM_NV12, 12, 12, CSS_FRAME_FORMAT_NV12, 0, CSS_FRAME_FORMAT_NV12 },
 	{ V4L2_MBUS_FMT_CUSTOM_NV21, 12, 12, CSS_FRAME_FORMAT_NV21, 0, CSS_FRAME_FORMAT_NV21 },
 	{ V4L2_MBUS_FMT_CUSTOM_YUV420, 12, 12, ATOMISP_INPUT_FORMAT_YUV420_8_LEGACY, 0, IA_CSS_STREAM_FORMAT_YUV420_8_LEGACY },
+#if 0
 	{ V4L2_MBUS_FMT_CUSTOM_M10MO_RAW, 8, 8, CSS_FRAME_FORMAT_BINARY_8, 0, IA_CSS_STREAM_FORMAT_BINARY_8 },
+#endif
 	/* no valid V4L2 MBUS code for metadata format, so leave it 0. */
 	{ 0, 0, 0, ATOMISP_INPUT_FORMAT_EMBEDDED, 0, IA_CSS_STREAM_FORMAT_EMBEDDED },
 	{}
-- 
2.15.1

[PATCH v1 05/10] staging: atomisp: Remove non-ACPI leftovers

[Andy Shevchenko]

Since all drivers are solely requiring ACPI enumeration, there is no
need to additionally check for legacy platform data or ACPI handle.

Remove leftovers from the sensors and platform code.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 10 ++---
 drivers/staging/media/atomisp/i2c/atomisp-gc2235.c |  8 +---
 drivers/staging/media/atomisp/i2c/atomisp-lm3554.c | 28 ++++----------
 .../staging/media/atomisp/i2c/atomisp-mt9m114.c    |  8 ++--
 drivers/staging/media/atomisp/i2c/atomisp-ov2680.c | 10 ++---
 drivers/staging/media/atomisp/i2c/atomisp-ov2722.c | 17 ++-------
 .../media/atomisp/i2c/ov5693/atomisp-ov5693.c      | 12 ++----
 drivers/staging/media/atomisp/i2c/ov8858.c         | 43 +++++++++++-----------
 .../platform/intel-mid/atomisp_gmin_platform.c     |  6 +--
 9 files changed, 49 insertions(+), 93 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index e70d8afcc229..61b7598469eb 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -1370,13 +1370,9 @@ static int gc0310_probe(struct i2c_client *client)
 	dev->fmt_idx = 0;
 	v4l2_i2c_subdev_init(&(dev->sd), client, &gc0310_ops);
 
-	if (ACPI_COMPANION(&client->dev))
-		pdata = gmin_camera_platform_data(&dev->sd,
-						  ATOMISP_INPUT_FORMAT_RAW_8,
-						  atomisp_bayer_order_grbg);
-	else
-		pdata = client->dev.platform_data;
-
+	pdata = gmin_camera_platform_data(&dev->sd,
+					  ATOMISP_INPUT_FORMAT_RAW_8,
+					  atomisp_bayer_order_grbg);
 	if (!pdata) {
 		ret = -EINVAL;
 		goto out_free;
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
index 85da5fe24033..d8de46da64ae 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc2235.c
@@ -1108,9 +1108,7 @@ static int gc2235_probe(struct i2c_client *client)
 	dev->fmt_idx = 0;
 	v4l2_i2c_subdev_init(&(dev->sd), client, &gc2235_ops);
 
-	gcpdev = client->dev.platform_data;
-	if (ACPI_COMPANION(&client->dev))
-		gcpdev = gmin_camera_platform_data(&dev->sd,
+	gcpdev = gmin_camera_platform_data(&dev->sd,
 				   ATOMISP_INPUT_FORMAT_RAW_10,
 				   atomisp_bayer_order_grbg);
 
@@ -1147,10 +1145,8 @@ static int gc2235_probe(struct i2c_client *client)
 	if (ret)
 		gc2235_remove(client);
 
-	if (ACPI_HANDLE(&client->dev))
-		ret = atomisp_register_i2c_module(&dev->sd, gcpdev, RAW_CAMERA);
+	return atomisp_register_i2c_module(&dev->sd, gcpdev, RAW_CAMERA);
 
-	return ret;
 out_free:
 	v4l2_device_unregister_subdev(&dev->sd);
 	kfree(dev);
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
index 974b6ff50c7a..7098bf317f16 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-lm3554.c
@@ -824,22 +824,15 @@ static void *lm3554_platform_data_func(struct i2c_client *client)
 {
 	static struct lm3554_platform_data platform_data;
 
-	if (ACPI_COMPANION(&client->dev)) {
-		platform_data.gpio_reset =
-		    desc_to_gpio(gpiod_get_index(&(client->dev),
+	platform_data.gpio_reset =
+		    desc_to_gpio(gpiod_get_index(&client->dev,
 						 NULL, 2, GPIOD_OUT_LOW));
-		platform_data.gpio_strobe =
-		    desc_to_gpio(gpiod_get_index(&(client->dev),
+	platform_data.gpio_strobe =
+		    desc_to_gpio(gpiod_get_index(&client->dev,
 						 NULL, 0, GPIOD_OUT_LOW));
-		platform_data.gpio_torch =
-		    desc_to_gpio(gpiod_get_index(&(client->dev),
+	platform_data.gpio_torch =
+		    desc_to_gpio(gpiod_get_index(&client->dev,
 						 NULL, 1, GPIOD_OUT_LOW));
-	} else {
-		platform_data.gpio_reset = -1;
-		platform_data.gpio_strobe = -1;
-		platform_data.gpio_torch = -1;
-	}
-
 	dev_info(&client->dev, "camera pdata: lm3554: reset: %d strobe %d torch %d\n",
 		platform_data.gpio_reset, platform_data.gpio_strobe,
 		platform_data.gpio_torch);
@@ -868,10 +861,7 @@ static int lm3554_probe(struct i2c_client *client)
 	if (!flash)
 		return -ENOMEM;
 
-	flash->pdata = client->dev.platform_data;
-
-	if (!flash->pdata || ACPI_COMPANION(&client->dev))
-		flash->pdata = lm3554_platform_data_func(client);
+	flash->pdata = lm3554_platform_data_func(client);
 
 	v4l2_i2c_subdev_init(&flash->sd, client, &lm3554_ops);
 	flash->sd.internal_ops = &lm3554_internal_ops;
@@ -914,9 +904,7 @@ static int lm3554_probe(struct i2c_client *client)
 		dev_err(&client->dev, "gpio request/direction_output fail");
 		goto fail2;
 	}
-	if (ACPI_HANDLE(&client->dev))
-		err = atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
-	return 0;
+	return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
 fail2:
 	media_entity_cleanup(&flash->sd.entity);
 	v4l2_ctrl_handler_free(&flash->ctrl_handler);
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c b/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c
index 55882bea2049..df253a557c76 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c
@@ -1844,11 +1844,9 @@ static int mt9m114_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	v4l2_i2c_subdev_init(&dev->sd, client, &mt9m114_ops);
-	pdata = client->dev.platform_data;
-	if (ACPI_COMPANION(&client->dev))
-		pdata = gmin_camera_platform_data(&dev->sd,
-						  ATOMISP_INPUT_FORMAT_RAW_10,
-						  atomisp_bayer_order_grbg);
+	pdata = gmin_camera_platform_data(&dev->sd,
+					  ATOMISP_INPUT_FORMAT_RAW_10,
+					  atomisp_bayer_order_grbg);
 	if (pdata)
 		ret = mt9m114_s_config(&dev->sd, client->irq, pdata);
 	if (!pdata || ret) {
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index cd67d38f183a..84f8d33ce2d1 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -1447,13 +1447,9 @@ static int ov2680_probe(struct i2c_client *client)
 	dev->fmt_idx = 0;
 	v4l2_i2c_subdev_init(&(dev->sd), client, &ov2680_ops);
 
-	if (ACPI_COMPANION(&client->dev))
-		pdata = gmin_camera_platform_data(&dev->sd,
-						  ATOMISP_INPUT_FORMAT_RAW_10,
-						  atomisp_bayer_order_bggr);
-	else
-		pdata = client->dev.platform_data;
-
+	pdata = gmin_camera_platform_data(&dev->sd,
+					  ATOMISP_INPUT_FORMAT_RAW_10,
+					  atomisp_bayer_order_bggr);
 	if (!pdata) {
 		ret = -EINVAL;
 		goto out_free;
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
index 4df7eba8d375..2b6ae0faf972 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2722.c
@@ -1259,7 +1259,6 @@ static int ov2722_probe(struct i2c_client *client)
 	struct ov2722_device *dev;
 	void *ovpdev;
 	int ret;
-	struct acpi_device *adev;
 
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 	if (!dev)
@@ -1270,14 +1269,9 @@ static int ov2722_probe(struct i2c_client *client)
 	dev->fmt_idx = 0;
 	v4l2_i2c_subdev_init(&(dev->sd), client, &ov2722_ops);
 
-	ovpdev = client->dev.platform_data;
-	adev = ACPI_COMPANION(&client->dev);
-	if (adev) {
-		adev->power.flags.power_resources = 0;
-		ovpdev = gmin_camera_platform_data(&dev->sd,
-						   ATOMISP_INPUT_FORMAT_RAW_10,
-						   atomisp_bayer_order_grbg);
-	}
+	ovpdev = gmin_camera_platform_data(&dev->sd,
+					   ATOMISP_INPUT_FORMAT_RAW_10,
+					   atomisp_bayer_order_grbg);
 
 	ret = ov2722_s_config(&dev->sd, client->irq, ovpdev);
 	if (ret)
@@ -1296,10 +1290,7 @@ static int ov2722_probe(struct i2c_client *client)
 	if (ret)
 		ov2722_remove(client);
 
-	if (ACPI_HANDLE(&client->dev))
-		ret = atomisp_register_i2c_module(&dev->sd, ovpdev, RAW_CAMERA);
-
-	return ret;
+	return atomisp_register_i2c_module(&dev->sd, ovpdev, RAW_CAMERA);
 
 out_ctrl_handler_free:
 	v4l2_ctrl_handler_free(&dev->ctrl_handler);
diff --git a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
index 3e7c3851280f..2a680eae20d3 100644
--- a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
+++ b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
@@ -1928,7 +1928,6 @@ static int ov5693_probe(struct i2c_client *client)
 	int i2c;
 	int ret = 0;
 	void *pdata = client->dev.platform_data;
-	struct acpi_device *adev;
 	unsigned int i;
 
 	/* Firmware workaround: Some modules use a "secondary default"
@@ -1952,14 +1951,9 @@ static int ov5693_probe(struct i2c_client *client)
 	dev->fmt_idx = 0;
 	v4l2_i2c_subdev_init(&(dev->sd), client, &ov5693_ops);
 
-	adev = ACPI_COMPANION(&client->dev);
-	if (adev) {
-		adev->power.flags.power_resources = 0;
-		pdata = gmin_camera_platform_data(&dev->sd,
-						  ATOMISP_INPUT_FORMAT_RAW_10,
-						  atomisp_bayer_order_bggr);
-	}
-
+	pdata = gmin_camera_platform_data(&dev->sd,
+					  ATOMISP_INPUT_FORMAT_RAW_10,
+					  atomisp_bayer_order_bggr);
 	if (!pdata)
 		goto out_free;
 
diff --git a/drivers/staging/media/atomisp/i2c/ov8858.c b/drivers/staging/media/atomisp/i2c/ov8858.c
index ba147ac2e36f..3cf8c710ac65 100644
--- a/drivers/staging/media/atomisp/i2c/ov8858.c
+++ b/drivers/staging/media/atomisp/i2c/ov8858.c
@@ -2077,29 +2077,28 @@ static int ov8858_probe(struct i2c_client *client)
 
 	v4l2_i2c_subdev_init(&(dev->sd), client, &ov8858_ops);
 
-	if (ACPI_COMPANION(&client->dev)) {
-		pdata = gmin_camera_platform_data(&dev->sd,
-						  ATOMISP_INPUT_FORMAT_RAW_10,
-						  atomisp_bayer_order_bggr);
-		if (!pdata) {
-			dev_err(&client->dev,
-				"%s: failed to get acpi platform data\n",
-				__func__);
-			goto out_free;
-		}
-		ret = ov8858_s_config(&dev->sd, client->irq, pdata);
-		if (ret) {
-			dev_err(&client->dev,
-				"%s: failed to set config\n", __func__);
-			goto out_free;
-		}
-		ret = atomisp_register_i2c_module(&dev->sd, pdata, RAW_CAMERA);
-		if (ret) {
-			dev_err(&client->dev,
-				"%s: failed to register subdev\n", __func__);
-			goto out_free;
-		}
+	pdata = gmin_camera_platform_data(&dev->sd,
+					  ATOMISP_INPUT_FORMAT_RAW_10,
+					  atomisp_bayer_order_bggr);
+	if (!pdata) {
+		dev_err(&client->dev,
+			"%s: failed to get acpi platform data\n",
+			__func__);
+		goto out_free;
+	}
+	ret = ov8858_s_config(&dev->sd, client->irq, pdata);
+	if (ret) {
+		dev_err(&client->dev,
+			"%s: failed to set config\n", __func__);
+		goto out_free;
 	}
+	ret = atomisp_register_i2c_module(&dev->sd, pdata, RAW_CAMERA);
+	if (ret) {
+		dev_err(&client->dev,
+			"%s: failed to register subdev\n", __func__);
+		goto out_free;
+	}
+
 	/*
 	 * sd->name is updated with sensor driver name by the v4l2.
 	 * change it to sensor name in this case.
diff --git a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
index 8fb5147531a5..8dcec0e780a1 100644
--- a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
@@ -114,7 +114,7 @@ int atomisp_register_i2c_module(struct v4l2_subdev *subdev,
 	struct i2c_board_info *bi;
 	struct gmin_subdev *gs;
 	struct i2c_client *client = v4l2_get_subdevdata(subdev);
-	struct acpi_device *adev;
+	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
 
 	dev_info(&client->dev, "register atomisp i2c module type %d\n", type);
 
@@ -124,9 +124,7 @@ int atomisp_register_i2c_module(struct v4l2_subdev *subdev,
 	 * tickled during suspend/resume.  This has caused power and
 	 * performance issues on multiple devices.
 	 */
-	adev = ACPI_COMPANION(&client->dev);
-	if (adev)
-		adev->power.flags.power_resources = 0;
+	adev->power.flags.power_resources = 0;
 
 	for (i = 0; i < MAX_SUBDEVS; i++)
 		if (!pdata.subdevs[i].type)
-- 
2.15.1

[PATCH v1 06/10] staging: atomisp: Switch to use struct device_driver directly

[Andy Shevchenko]

In a preparation of split PCI glue driver from core part, convert
the driver to use more generic struct device_driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 .../staging/media/atomisp/pci/atomisp2/atomisp_drvfs.c  | 17 ++++++++---------
 .../staging/media/atomisp/pci/atomisp2/atomisp_drvfs.h  |  5 ++---
 .../staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c   |  4 +---
 3 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_drvfs.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_drvfs.c
index 7129b88456cb..ceedb82b6beb 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_drvfs.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_drvfs.c
@@ -15,9 +15,9 @@
  *
  */
 
+#include <linux/device.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
-#include <linux/pci.h>
 
 #include "atomisp_compat.h"
 #include "atomisp_internal.h"
@@ -33,7 +33,7 @@
  *        bit 2: memory statistic
 */
 struct _iunit_debug {
-	struct pci_driver	*drv;
+	struct device_driver	*drv;
 	struct atomisp_device	*isp;
 	unsigned int		dbglvl;
 	unsigned int		dbgfun;
@@ -164,26 +164,25 @@ static const struct driver_attribute iunit_drvfs_attrs[] = {
 	__ATTR(dbgopt, 0644, iunit_dbgopt_show, iunit_dbgopt_store),
 };
 
-static int iunit_drvfs_create_files(struct pci_driver *drv)
+static int iunit_drvfs_create_files(struct device_driver *drv)
 {
 	int i, ret = 0;
 
 	for (i = 0; i < ARRAY_SIZE(iunit_drvfs_attrs); i++)
-		ret |= driver_create_file(&(drv->driver),
-					&iunit_drvfs_attrs[i]);
+		ret |= driver_create_file(drv, &iunit_drvfs_attrs[i]);
 
 	return ret;
 }
 
-static void iunit_drvfs_remove_files(struct pci_driver *drv)
+static void iunit_drvfs_remove_files(struct device_driver *drv)
 {
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(iunit_drvfs_attrs); i++)
-		driver_remove_file(&(drv->driver), &iunit_drvfs_attrs[i]);
+		driver_remove_file(drv, &iunit_drvfs_attrs[i]);
 }
 
-int atomisp_drvfs_init(struct pci_driver *drv, struct atomisp_device *isp)
+int atomisp_drvfs_init(struct device_driver *drv, struct atomisp_device *isp)
 {
 	int ret;
 
@@ -193,7 +192,7 @@ int atomisp_drvfs_init(struct pci_driver *drv, struct atomisp_device *isp)
 	ret = iunit_drvfs_create_files(iunit_debug.drv);
 	if (ret) {
 		dev_err(atomisp_dev, "drvfs_create_files error: %d\n", ret);
-		iunit_drvfs_remove_files(drv);
+		iunit_drvfs_remove_files(iunit_debug.drv);
 	}
 
 	return ret;
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_drvfs.h b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_drvfs.h
index b91bfef21639..7c99240d107a 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_drvfs.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_drvfs.h
@@ -18,8 +18,7 @@
 #ifndef	__ATOMISP_DRVFS_H__
 #define	__ATOMISP_DRVFS_H__
 
-extern int atomisp_drvfs_init(struct pci_driver *drv, struct atomisp_device
-				*isp);
-extern void atomisp_drvfs_exit(void);
+int atomisp_drvfs_init(struct device_driver *drv, struct atomisp_device *isp);
+void atomisp_drvfs_exit(void);
 
 #endif /* __ATOMISP_DRVFS_H__ */
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
index 3c260f8b52e2..7a9efc6847ca 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
@@ -1152,8 +1152,6 @@ static int init_atomisp_wdts(struct atomisp_device *isp)
 	return err;
 }
 
-static struct pci_driver atomisp_pci_driver;
-
 #define ATOM_ISP_PCI_BAR	0
 
 static int atomisp_pci_probe(struct pci_dev *dev,
@@ -1451,7 +1449,7 @@ static int atomisp_pci_probe(struct pci_dev *dev,
 	isp->firmware = NULL;
 	isp->css_env.isp_css_fw.data = NULL;
 
-	atomisp_drvfs_init(&atomisp_pci_driver, isp);
+	atomisp_drvfs_init(&dev->driver->driver, isp);
 
 	return 0;
 
-- 
2.15.1

[PATCH v1 07/10] staging: atomisp: Remove redundant PCI code

[Andy Shevchenko]

There is no need to keep a reference to PCI root bridge.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h | 1 -
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c     | 8 --------
 2 files changed, 9 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h
index 52a6f8002048..dc476a3dd271 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h
@@ -227,7 +227,6 @@ struct atomisp_device {
 	struct media_device media_dev;
 	struct atomisp_platform_data *pdata;
 	void *mmu_l1_base;
-	struct pci_dev *pci_root;
 	const struct firmware *firmware;
 
 	struct pm_qos_request pm_qos;
diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
index 7a9efc6847ca..548e00e7d67b 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c
@@ -1210,11 +1210,6 @@ static int atomisp_pci_probe(struct pci_dev *dev,
 	isp->pdev = dev;
 	isp->dev = &dev->dev;
 	isp->sw_contex.power_state = ATOM_ISP_POWER_UP;
-	isp->pci_root = pci_get_bus_and_slot(0, 0);
-	if (!isp->pci_root) {
-		dev_err(&dev->dev, "Unable to find PCI host\n");
-		return -ENODEV;
-	}
 	isp->saved_regs.ispmmadr = start;
 
 	rt_mutex_init(&isp->mutex);
@@ -1494,7 +1489,6 @@ static int atomisp_pci_probe(struct pci_dev *dev,
 	/* Address later when we worry about the ...field chips */
 	if (IS_ENABLED(CONFIG_PM) && atomisp_mrfld_power_down(isp))
 		dev_err(&dev->dev, "Failed to switch off ISP\n");
-	pci_dev_put(isp->pci_root);
 	return err;
 }
 
@@ -1515,8 +1509,6 @@ static void atomisp_pci_remove(struct pci_dev *dev)
 	pm_qos_remove_request(&isp->pm_qos);
 
 	atomisp_msi_irq_uninit(isp, dev);
-	pci_dev_put(isp->pci_root);
-
 	atomisp_unregister_entities(isp);
 
 	destroy_workqueue(isp->wdt_work_queue);
-- 
2.15.1

[PATCH v1 08/10] staging: atomisp: Unexport local function

[Andy Shevchenko]

There is no need to export function which is only used once in
the same module where it's defined.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/staging/media/atomisp/include/linux/atomisp_gmin_platform.h  | 1 -
 .../staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c | 5 ++---
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/atomisp/include/linux/atomisp_gmin_platform.h b/drivers/staging/media/atomisp/include/linux/atomisp_gmin_platform.h
index 7e3ca12dd4e9..c52c56a17e17 100644
--- a/drivers/staging/media/atomisp/include/linux/atomisp_gmin_platform.h
+++ b/drivers/staging/media/atomisp/include/linux/atomisp_gmin_platform.h
@@ -23,7 +23,6 @@ int atomisp_register_i2c_module(struct v4l2_subdev *subdev,
 struct v4l2_subdev *atomisp_gmin_find_subdev(struct i2c_adapter *adapter,
 					     struct i2c_board_info *board_info);
 int atomisp_gmin_remove_subdev(struct v4l2_subdev *sd);
-int gmin_get_config_var(struct device *dev, const char *var, char *out, size_t *out_len);
 int gmin_get_var_int(struct device *dev, const char *var, int def);
 int camera_sensor_csi(struct v4l2_subdev *sd, u32 port,
                       u32 lanes, u32 format, u32 bayer_order, int flag);
diff --git a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
index 8dcec0e780a1..0f859bb714bf 100644
--- a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
@@ -608,8 +608,8 @@ EXPORT_SYMBOL_GPL(atomisp_gmin_register_vcm_control);
  * argument should be a device with an ACPI companion, as all
  * configuration is based on firmware ID.
  */
-int gmin_get_config_var(struct device *dev, const char *var, char *out,
-			size_t *out_len)
+static int gmin_get_config_var(struct device *dev, const char *var,
+			       char *out, size_t *out_len)
 {
 	char var8[CFG_VAR_NAME_MAX];
 	efi_char16_t var16[CFG_VAR_NAME_MAX];
@@ -691,7 +691,6 @@ int gmin_get_config_var(struct device *dev, const char *var, char *out,
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(gmin_get_config_var);
 
 int gmin_get_var_int(struct device *dev, const char *var, int def)
 {
-- 
2.15.1

[PATCH v1 09/10] staging: atomisp: Use standard DMI match table

[Andy Shevchenko]

The traditional pattern is to use DMI matching table and provide a
corresponding driver_data in it.

Convert driver to use DMI matching table.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 .../platform/intel-mid/atomisp_gmin_platform.c     | 109 +++++++++++++--------
 1 file changed, 70 insertions(+), 39 deletions(-)

diff --git a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
index 0f859bb714bf..8408a58ed764 100644
--- a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
@@ -209,7 +209,7 @@ struct gmin_cfg_var {
 	const char *name, *val;
 };
 
-static const struct gmin_cfg_var ffrd8_vars[] = {
+static struct gmin_cfg_var ffrd8_vars[] = {
 	{ "INTCF1B:00_ImxId",    "0x134" },
 	{ "INTCF1B:00_CsiPort",  "1" },
 	{ "INTCF1B:00_CsiLanes", "4" },
@@ -220,14 +220,14 @@ static const struct gmin_cfg_var ffrd8_vars[] = {
 /* Cribbed from MCG defaults in the mt9m114 driver, not actually verified
  * vs. T100 hardware
  */
-static const struct gmin_cfg_var t100_vars[] = {
+static struct gmin_cfg_var t100_vars[] = {
 	{ "INT33F0:00_CsiPort",  "0" },
 	{ "INT33F0:00_CsiLanes", "1" },
 	{ "INT33F0:00_CamClk",   "1" },
 	{},
 };
 
-static const struct gmin_cfg_var mrd7_vars[] = {
+static struct gmin_cfg_var mrd7_vars[] = {
 	{"INT33F8:00_CamType", "1"},
 	{"INT33F8:00_CsiPort", "1"},
 	{"INT33F8:00_CsiLanes", "2"},
@@ -243,7 +243,7 @@ static const struct gmin_cfg_var mrd7_vars[] = {
 	{},
 };
 
-static const struct gmin_cfg_var ecs7_vars[] = {
+static struct gmin_cfg_var ecs7_vars[] = {
 	{"INT33BE:00_CsiPort", "1"},
 	{"INT33BE:00_CsiLanes", "2"},
 	{"INT33BE:00_CsiFmt", "13"},
@@ -258,8 +258,7 @@ static const struct gmin_cfg_var ecs7_vars[] = {
 	{},
 };
 
-
-static const struct gmin_cfg_var i8880_vars[] = {
+static struct gmin_cfg_var i8880_vars[] = {
 	{"XXOV2680:00_CsiPort", "1"},
 	{"XXOV2680:00_CsiLanes", "1"},
 	{"XXOV2680:00_CamClk", "0"},
@@ -269,18 +268,45 @@ static const struct gmin_cfg_var i8880_vars[] = {
 	{},
 };
 
-static const struct {
-	const char *dmi_board_name;
-	const struct gmin_cfg_var *vars;
-} hard_vars[] = {
-	{ "BYT-T FFD8", ffrd8_vars },
-	{ "T100TA", t100_vars },
-	{ "MRD7", mrd7_vars },
-	{ "ST70408", ecs7_vars },
-	{ "VTA0803", i8880_vars },
+static const struct dmi_system_id gmin_vars[] = {
+	{
+		.ident = "BYT-T FFD8",
+		.matches = {
+			DMI_MATCH(DMI_BOARD_NAME, "BYT-T FFD8"),
+		},
+		.driver_data = ffrd8_vars,
+	},
+	{
+		.ident = "T100TA",
+		.matches = {
+			DMI_MATCH(DMI_BOARD_NAME, "T100TA"),
+		},
+		.driver_data = t100_vars,
+	},
+	{
+		.ident = "MRD7",
+		.matches = {
+			DMI_MATCH(DMI_BOARD_NAME, "MRD7"),
+		},
+		.driver_data = mrd7_vars,
+	},
+	{
+		.ident = "ST70408",
+		.matches = {
+			DMI_MATCH(DMI_BOARD_NAME, "ST70408"),
+		},
+		.driver_data = ecs7_vars,
+	},
+	{
+		.ident = "VTA0803",
+		.matches = {
+			DMI_MATCH(DMI_BOARD_NAME, "VTA0803"),
+		},
+		.driver_data = i8880_vars,
+	},
+	{}
 };
 
-
 #define GMIN_CFG_VAR_EFI_GUID EFI_GUID(0xecb54cd9, 0xe5ae, 0x4fdc, \
 				       0xa9, 0x71, 0xe8, 0x77,	   \
 				       0x75, 0x60, 0x68, 0xf7)
@@ -604,6 +630,29 @@ int atomisp_gmin_register_vcm_control(struct camera_vcm_control *vcmCtrl)
 }
 EXPORT_SYMBOL_GPL(atomisp_gmin_register_vcm_control);
 
+static int gmin_get_hardcoded_var(struct gmin_cfg_var *varlist,
+				  const char *var8, char *out, size_t *out_len)
+{
+	struct gmin_cfg_var *gv;
+
+	for (gv = varlist; gv->name; gv++) {
+		size_t vl;
+
+		if (strcmp(var8, gv->name))
+			continue;
+
+		vl = strlen(gv->val);
+		if (vl > *out_len - 1)
+			return -ENOSPC;
+
+		strcpy(out, gv->val);
+		*out_len = vl;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 /* Retrieves a device-specific configuration variable.  The dev
  * argument should be a device with an ACPI companion, as all
  * configuration is based on firmware ID.
@@ -614,7 +663,8 @@ static int gmin_get_config_var(struct device *dev, const char *var,
 	char var8[CFG_VAR_NAME_MAX];
 	efi_char16_t var16[CFG_VAR_NAME_MAX];
 	struct efivar_entry *ev;
-	int i, j, ret;
+	const struct dmi_system_id *id;
+	int i, ret;
 
 	if (dev && ACPI_COMPANION(dev))
 		dev = &ACPI_COMPANION(dev)->dev;
@@ -631,28 +681,9 @@ static int gmin_get_config_var(struct device *dev, const char *var,
 	 * Some device firmwares lack the ability to set EFI variables at
 	 * runtime.
 	 */
-	for (i = 0; i < ARRAY_SIZE(hard_vars); i++) {
-		if (dmi_match(DMI_BOARD_NAME, hard_vars[i].dmi_board_name)) {
-			for (j = 0; hard_vars[i].vars[j].name; j++) {
-				size_t vl;
-				const struct gmin_cfg_var *gv;
-
-				gv = &hard_vars[i].vars[j];
-				vl = strlen(gv->val);
-
-				if (strcmp(var8, gv->name))
-					continue;
-				if (vl > *out_len - 1)
-					return -ENOSPC;
-
-				memcpy(out, gv->val, min(*out_len, vl+1));
-				out[*out_len-1] = 0;
-				*out_len = vl;
-
-				return 0;
-			}
-		}
-	}
+	id = dmi_first_match(gmin_vars);
+	if (id)
+		return gmin_get_hardcoded_var(id->driver_data, var8, out, out_len);
 
 	/* Our variable names are ASCII by construction, but EFI names
 	 * are wide chars.  Convert and zero-pad.
-- 
2.15.1

[PATCH v1 10/10] staging: atomisp: Fix DMI matching entry for MRD7

[Andy Shevchenko]

MRD7 board has in particular

	Base Board Information
		Manufacturer: Intel Corp.
		Product Name: TABLET
		Version: MRD 7

Fix the DMI matching entry for it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 .../staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c   | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
index 8408a58ed764..d8b7183db252 100644
--- a/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c
@@ -286,7 +286,8 @@ static const struct dmi_system_id gmin_vars[] = {
 	{
 		.ident = "MRD7",
 		.matches = {
-			DMI_MATCH(DMI_BOARD_NAME, "MRD7"),
+			DMI_MATCH(DMI_BOARD_NAME, "TABLET"),
+			DMI_MATCH(DMI_BOARD_VERSION, "MRD 7"),
 		},
 		.driver_data = mrd7_vars,
 	},
-- 
2.15.1

Re: [PATCH v1 05/10] staging: atomisp: Remove non-ACPI leftovers

[Dan Carpenter]

On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote:
> @@ -1147,10 +1145,8 @@ static int gc2235_probe(struct i2c_client *client)
>  	if (ret)
>  		gc2235_remove(client);

This error handling is probably wrong...

>  
> -	if (ACPI_HANDLE(&client->dev))
> -		ret = atomisp_register_i2c_module(&dev->sd, gcpdev, RAW_CAMERA);
> +	return atomisp_register_i2c_module(&dev->sd, gcpdev, RAW_CAMERA);

In the end this should look something like:

	ret = atomisp_register_i2c_module(&dev->sd, gcpdev, RAW_CAMERA);
	if (ret)
		goto err_free_something;

	return 0;

>  
> -	return ret;
>  out_free:
>  	v4l2_device_unregister_subdev(&dev->sd);
>  	kfree(dev);

regards,
dan carpenter

Re: [PATCH v1 05/10] staging: atomisp: Remove non-ACPI leftovers

[Dan Carpenter]

On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote:
> @@ -914,9 +904,7 @@ static int lm3554_probe(struct i2c_client *client)
>  		dev_err(&client->dev, "gpio request/direction_output fail");
>  		goto fail2;
>  	}
> -	if (ACPI_HANDLE(&client->dev))
> -		err = atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
> -	return 0;
> +	return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
>  fail2:
>  	media_entity_cleanup(&flash->sd.entity);
>  	v4l2_ctrl_handler_free(&flash->ctrl_handler);

Actually every place where we directly return a function call is wrong
and needs error handling added.  I've been meaning to write a Smatch
check for this because it's a common anti-pattern we don't check the
last function call for errors.

Someone could probably do the same in Coccinelle if they want.

regards,
dan carpenter

Re: [PATCH v1 05/10] staging: atomisp: Remove non-ACPI leftovers

[Dan Carpenter]

On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote:
> @@ -914,9 +904,7 @@ static int lm3554_probe(struct i2c_client *client)
>  		dev_err(&client->dev, "gpio request/direction_output fail");
>  		goto fail2;
>  	}
> -	if (ACPI_HANDLE(&client->dev))
> -		err = atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
> -	return 0;
> +	return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
>  fail2:
>  	media_entity_cleanup(&flash->sd.entity);
>  	v4l2_ctrl_handler_free(&flash->ctrl_handler);

Actually every place where we directly return a function call is wrong
and needs error handling added.  I've been meaning to write a Smatch
check for this because it's a common anti-pattern we don't check the
last function call for errors.

Someone could probably do the same in Coccinelle if they want.

regards,
dan carpenter

Re: [PATCH v1 05/10] staging: atomisp: Remove non-ACPI leftovers

[Andy Shevchenko]

On Wed, Dec 20, 2017 at 6:54 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote:
>> @@ -1147,10 +1145,8 @@ static int gc2235_probe(struct i2c_client *client)
>>       if (ret)
>>               gc2235_remove(client);
>
> This error handling is probably wrong...
>

Thanks for pointing to this, but I'm not going to fix this by the
following reasons:
1. I admit the driver's code is ugly
2. It's staging code
3. My patch does not touch those lines
4. My purpose is to get it working first.

Feel free to send a followup with a good clean up which I agree with.

>>
>> -     if (ACPI_HANDLE(&client->dev))
>> -             ret = atomisp_register_i2c_module(&dev->sd, gcpdev, RAW_CAMERA);
>> +     return atomisp_register_i2c_module(&dev->sd, gcpdev, RAW_CAMERA);
>
> In the end this should look something like:
>
>         ret = atomisp_register_i2c_module(&dev->sd, gcpdev, RAW_CAMERA);
>         if (ret)
>                 goto err_free_something;
>
>         return 0;
>
>>
>> -     return ret;
>>  out_free:
>>       v4l2_device_unregister_subdev(&dev->sd);
>>       kfree(dev);
>
> regards,
> dan carpenter
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 05/10] staging: atomisp: Remove non-ACPI leftovers

[Julia Lawall]

On Wed, 20 Dec 2017, Dan Carpenter wrote:

> On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote:
> > @@ -914,9 +904,7 @@ static int lm3554_probe(struct i2c_client *client)
> >  		dev_err(&client->dev, "gpio request/direction_output fail");
> >  		goto fail2;
> >  	}
> > -	if (ACPI_HANDLE(&client->dev))
> > -		err = atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
> > -	return 0;
> > +	return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
> >  fail2:
> >  	media_entity_cleanup(&flash->sd.entity);
> >  	v4l2_ctrl_handler_free(&flash->ctrl_handler);
>
> Actually every place where we directly return a function call is wrong
> and needs error handling added.  I've been meaning to write a Smatch
> check for this because it's a common anti-pattern we don't check the
> last function call for errors.
>
> Someone could probably do the same in Coccinelle if they want.

I'm not sure what you are suggesting.  Is every case of return f(...);
for any f wrong?  Or is it a particular function that is of concern?  Or
would it be that every function call that has error handling somewhere
should have error handling everywhere?  Or is it related to what seems to
be the problem in the above code that err is initialized but nothing
happens to it?

julia

Re: [PATCH v1 05/10] staging: atomisp: Remove non-ACPI leftovers

[Julia Lawall]

On Wed, 20 Dec 2017, Dan Carpenter wrote:

> On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote:
> > @@ -914,9 +904,7 @@ static int lm3554_probe(struct i2c_client *client)
> >  		dev_err(&client->dev, "gpio request/direction_output fail");
> >  		goto fail2;
> >  	}
> > -	if (ACPI_HANDLE(&client->dev))
> > -		err = atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
> > -	return 0;
> > +	return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
> >  fail2:
> >  	media_entity_cleanup(&flash->sd.entity);
> >  	v4l2_ctrl_handler_free(&flash->ctrl_handler);
>
> Actually every place where we directly return a function call is wrong
> and needs error handling added.  I've been meaning to write a Smatch
> check for this because it's a common anti-pattern we don't check the
> last function call for errors.
>
> Someone could probably do the same in Coccinelle if they want.

I'm not sure what you are suggesting.  Is every case of return f(...);
for any f wrong?  Or is it a particular function that is of concern?  Or
would it be that every function call that has error handling somewhere
should have error handling everywhere?  Or is it related to what seems to
be the problem in the above code that err is initialized but nothing
happens to it?

julia

Re: [PATCH v1 05/10] staging: atomisp: Remove non-ACPI leftovers

[walter harms]

Am 20.12.2017 11:30, schrieb Julia Lawall:
> 
> 
> On Wed, 20 Dec 2017, Dan Carpenter wrote:
> 
>> On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote:
>>> @@ -914,9 +904,7 @@ static int lm3554_probe(struct i2c_client *client)
>>>  		dev_err(&client->dev, "gpio request/direction_output fail");
>>>  		goto fail2;
>>>  	}
>>> -	if (ACPI_HANDLE(&client->dev))
>>> -		err = atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
>>> -	return 0;
>>> +	return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
>>>  fail2:
>>>  	media_entity_cleanup(&flash->sd.entity);
>>>  	v4l2_ctrl_handler_free(&flash->ctrl_handler);
>>
>> Actually every place where we directly return a function call is wrong
>> and needs error handling added.  I've been meaning to write a Smatch
>> check for this because it's a common anti-pattern we don't check the
>> last function call for errors.
>>
>> Someone could probably do the same in Coccinelle if they want.
> 
> I'm not sure what you are suggesting.  Is every case of return f(...);
> for any f wrong?  Or is it a particular function that is of concern?  Or
> would it be that every function call that has error handling somewhere
> should have error handling everywhere?  Or is it related to what seems to
> be the problem in the above code that err is initialized but nothing
> happens to it?
> 

I guess the idea is to check if a return value gets set and then discarded
because the function returns const.

IMHO this is a case of write-never read like that series what Colin King fixed lately.

re,
 wh

Re: [PATCH v1 05/10] staging: atomisp: Remove non-ACPI leftovers

[Julia Lawall]

On Wed, 20 Dec 2017, walter harms wrote:

>
>
> Am 20.12.2017 11:30, schrieb Julia Lawall:
> >
> >
> > On Wed, 20 Dec 2017, Dan Carpenter wrote:
> >
> >> On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote:
> >>> @@ -914,9 +904,7 @@ static int lm3554_probe(struct i2c_client *client)
> >>>  		dev_err(&client->dev, "gpio request/direction_output fail");
> >>>  		goto fail2;
> >>>  	}
> >>> -	if (ACPI_HANDLE(&client->dev))
> >>> -		err = atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
> >>> -	return 0;
> >>> +	return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
> >>>  fail2:
> >>>  	media_entity_cleanup(&flash->sd.entity);
> >>>  	v4l2_ctrl_handler_free(&flash->ctrl_handler);
> >>
> >> Actually every place where we directly return a function call is wrong
> >> and needs error handling added.  I've been meaning to write a Smatch
> >> check for this because it's a common anti-pattern we don't check the
> >> last function call for errors.
> >>
> >> Someone could probably do the same in Coccinelle if they want.
> >
> > I'm not sure what you are suggesting.  Is every case of return f(...);
> > for any f wrong?  Or is it a particular function that is of concern?  Or
> > would it be that every function call that has error handling somewhere
> > should have error handling everywhere?  Or is it related to what seems to
> > be the problem in the above code that err is initialized but nothing
> > happens to it?
> >
>
> I guess the idea is to check if a return value gets set and then discarded
> because the function returns const.
>
> IMHO this is a case of write-never read like that series what Colin King fixed lately.

OK, that makes sense.  I guess Colin's patches were in the case where the
right hand side of the assignment is a constant, so non-side effecting.
This case is a bit different.  In particular, one might want to skip cases
where the return value is stored but not tested, because the developer
knows that the value is not interesting.  But cases where the variable
holding the result of a call is returned as the final result in some cases
but not in the current case could be worth looking at.

At the other end, there are probably a lot of cases of, eg

ty x = get_x(...);

that have been reflexively put at the beginning of functions, because all
other functions have it, but where in the current case x is not used.

julia


>
> re,
>  wh
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v1 05/10] staging: atomisp: Remove non-ACPI leftovers

[Sakari Ailus]

Hi Andy and Dan,

On Wed, Dec 20, 2017 at 12:24:36PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 20, 2017 at 6:54 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote:
> >> @@ -1147,10 +1145,8 @@ static int gc2235_probe(struct i2c_client *client)
> >>       if (ret)
> >>               gc2235_remove(client);
> >
> > This error handling is probably wrong...
> >
> 
> Thanks for pointing to this, but I'm not going to fix this by the
> following reasons:
> 1. I admit the driver's code is ugly
> 2. It's staging code
> 3. My patch does not touch those lines
> 4. My purpose is to get it working first.
> 
> Feel free to send a followup with a good clean up which I agree with.

Yeah, there's a lot of ugly stuff in this driver... I understand Andy's
patches address problems with functionality, let's make error handling
fixes separately.

So I'm applying these now.

Thanks!

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v1 05/10] staging: atomisp: Remove non-ACPI leftovers

[Dan Carpenter]

On Wed, Dec 20, 2017 at 11:30:01AM +0100, Julia Lawall wrote:
> 
> 
> On Wed, 20 Dec 2017, Dan Carpenter wrote:
> 
> > On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote:
> > > @@ -914,9 +904,7 @@ static int lm3554_probe(struct i2c_client *client)
> > >  		dev_err(&client->dev, "gpio request/direction_output fail");
> > >  		goto fail2;
> > >  	}
> > > -	if (ACPI_HANDLE(&client->dev))
> > > -		err = atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
> > > -	return 0;
> > > +	return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
> > >  fail2:
> > >  	media_entity_cleanup(&flash->sd.entity);
> > >  	v4l2_ctrl_handler_free(&flash->ctrl_handler);
> >
> > Actually every place where we directly return a function call is wrong
> > and needs error handling added.  I've been meaning to write a Smatch
> > check for this because it's a common anti-pattern we don't check the
> > last function call for errors.
> >
> > Someone could probably do the same in Coccinelle if they want.
> 
> I'm not sure what you are suggesting.  Is every case of return f(...);
> for any f wrong?  Or is it a particular function that is of concern?  Or
> would it be that every function call that has error handling somewhere
> should have error handling everywhere?  Or is it related to what seems to
> be the problem in the above code that err is initialized but nothing
> happens to it?
> 

I was just thinking that it's a common pattern to treat the last
function call differently and one mistake I often see looks like this:

	ret = frob();
	if (ret) {
		cleanup();
		return ret;
	}

	return another_function();

No error handling for the last function call.

regards,
dan carpenter

Re: [PATCH v1 05/10] staging: atomisp: Remove non-ACPI leftovers

[Dan Carpenter]

On Wed, Dec 20, 2017 at 11:30:01AM +0100, Julia Lawall wrote:
> 
> 
> On Wed, 20 Dec 2017, Dan Carpenter wrote:
> 
> > On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote:
> > > @@ -914,9 +904,7 @@ static int lm3554_probe(struct i2c_client *client)
> > >  		dev_err(&client->dev, "gpio request/direction_output fail");
> > >  		goto fail2;
> > >  	}
> > > -	if (ACPI_HANDLE(&client->dev))
> > > -		err = atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
> > > -	return 0;
> > > +	return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
> > >  fail2:
> > >  	media_entity_cleanup(&flash->sd.entity);
> > >  	v4l2_ctrl_handler_free(&flash->ctrl_handler);
> >
> > Actually every place where we directly return a function call is wrong
> > and needs error handling added.  I've been meaning to write a Smatch
> > check for this because it's a common anti-pattern we don't check the
> > last function call for errors.
> >
> > Someone could probably do the same in Coccinelle if they want.
> 
> I'm not sure what you are suggesting.  Is every case of return f(...);
> for any f wrong?  Or is it a particular function that is of concern?  Or
> would it be that every function call that has error handling somewhere
> should have error handling everywhere?  Or is it related to what seems to
> be the problem in the above code that err is initialized but nothing
> happens to it?
> 

I was just thinking that it's a common pattern to treat the last
function call differently and one mistake I often see looks like this:

	ret = frob();
	if (ret) {
		cleanup();
		return ret;
	}

	return another_function();

No error handling for the last function call.

regards,
dan carpenter

Re: [PATCH v1 05/10] staging: atomisp: Remove non-ACPI leftovers

[Julia Lawall]

On Tue, 2 Jan 2018, Dan Carpenter wrote:

> On Wed, Dec 20, 2017 at 11:30:01AM +0100, Julia Lawall wrote:
> >
> >
> > On Wed, 20 Dec 2017, Dan Carpenter wrote:
> >
> > > On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote:
> > > > @@ -914,9 +904,7 @@ static int lm3554_probe(struct i2c_client *client)
> > > >  		dev_err(&client->dev, "gpio request/direction_output fail");
> > > >  		goto fail2;
> > > >  	}
> > > > -	if (ACPI_HANDLE(&client->dev))
> > > > -		err = atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
> > > > -	return 0;
> > > > +	return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
> > > >  fail2:
> > > >  	media_entity_cleanup(&flash->sd.entity);
> > > >  	v4l2_ctrl_handler_free(&flash->ctrl_handler);
> > >
> > > Actually every place where we directly return a function call is wrong
> > > and needs error handling added.  I've been meaning to write a Smatch
> > > check for this because it's a common anti-pattern we don't check the
> > > last function call for errors.
> > >
> > > Someone could probably do the same in Coccinelle if they want.
> >
> > I'm not sure what you are suggesting.  Is every case of return f(...);
> > for any f wrong?  Or is it a particular function that is of concern?  Or
> > would it be that every function call that has error handling somewhere
> > should have error handling everywhere?  Or is it related to what seems to
> > be the problem in the above code that err is initialized but nothing
> > happens to it?
> >
>
> I was just thinking that it's a common pattern to treat the last
> function call differently and one mistake I often see looks like this:
>
> 	ret = frob();
> 	if (ret) {
> 		cleanup();
> 		return ret;
> 	}
>
> 	return another_function();
>
> No error handling for the last function call.

OK, I see.  When there was error handling code along the way, a direct
return of a function that could fail needs error handling code too.

Thanks for the clarification,

julia

Re: [PATCH v1 05/10] staging: atomisp: Remove non-ACPI leftovers

[Julia Lawall]

On Tue, 2 Jan 2018, Dan Carpenter wrote:

> On Wed, Dec 20, 2017 at 11:30:01AM +0100, Julia Lawall wrote:
> >
> >
> > On Wed, 20 Dec 2017, Dan Carpenter wrote:
> >
> > > On Tue, Dec 19, 2017 at 10:59:52PM +0200, Andy Shevchenko wrote:
> > > > @@ -914,9 +904,7 @@ static int lm3554_probe(struct i2c_client *client)
> > > >  		dev_err(&client->dev, "gpio request/direction_output fail");
> > > >  		goto fail2;
> > > >  	}
> > > > -	if (ACPI_HANDLE(&client->dev))
> > > > -		err = atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
> > > > -	return 0;
> > > > +	return atomisp_register_i2c_module(&flash->sd, NULL, LED_FLASH);
> > > >  fail2:
> > > >  	media_entity_cleanup(&flash->sd.entity);
> > > >  	v4l2_ctrl_handler_free(&flash->ctrl_handler);
> > >
> > > Actually every place where we directly return a function call is wrong
> > > and needs error handling added.  I've been meaning to write a Smatch
> > > check for this because it's a common anti-pattern we don't check the
> > > last function call for errors.
> > >
> > > Someone could probably do the same in Coccinelle if they want.
> >
> > I'm not sure what you are suggesting.  Is every case of return f(...);
> > for any f wrong?  Or is it a particular function that is of concern?  Or
> > would it be that every function call that has error handling somewhere
> > should have error handling everywhere?  Or is it related to what seems to
> > be the problem in the above code that err is initialized but nothing
> > happens to it?
> >
>
> I was just thinking that it's a common pattern to treat the last
> function call differently and one mistake I often see looks like this:
>
> 	ret = frob();
> 	if (ret) {
> 		cleanup();
> 		return ret;
> 	}
>
> 	return another_function();
>
> No error handling for the last function call.

OK, I see.  When there was error handling code along the way, a direct
return of a function that could fail needs error handling code too.

Thanks for the clarification,

julia