staging: atomisp: Don't leak GPIO resources if clk_get() failed

PATCH In review Andy Shevchenko 6 participants 19 messages Dec 19, 2017 – Jan 2, 2018

Subsystems (5)

Media (V4L/DVB) Staging Kernel janitors Intel MID (Mobile Internet Device) platform +1 more subsystems

lore

Share & tools

lore

Andy Shevchenko

author

2017-12-19 21:08 UTC #0 lore raw

PATCH 1/10 staging: atomisp: Don't leak GPIO resources if clk_get() failed

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 <[email protected]>

---
 .../staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c  | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c +2 -2

index bf9f34b7ad72..a5d0dd88a8bc 100644
@@ -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;

Signature · 1 line

2.15.1

Andy Shevchenko

author

2017-12-19 21:00 UTC #1 lore raw

PATCH 2/10 staging: atomisp: Remove duplicate NULL-check

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

Signed-off-by Andy Shevchenko <[email protected]>

---
 .../staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c  | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c +2 -2

index a5d0dd88a8bc..8fb5147531a5 100644
@@ -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;
 	}

Signature · 1 line

2.15.1

Andy Shevchenko

author

2017-12-19 21:00 UTC #2 lore raw

PATCH 3/10 staging: atomisp: lm3554: Fix control values

In reply to 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 <[email protected]>

---
 drivers/staging/media/atomisp/i2c/atomisp-lm3554.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

drivers/staging/media/atomisp/i2c/atomisp-lm3554.c +5 -5

index 4fd9f538ac95..974b6ff50c7a 100644
@@ -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,

Signature · 1 line

2.15.1

Andy Shevchenko

author

2017-12-19 21:01 UTC #3 lore raw

PATCH 4/10 staging: atomisp: Disable custom format for now

In reply to 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 <[email protected]>

---
 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(-)

drivers/staging/media/atomisp/include/linux/atomisp.h +2 -0

index 15fa5679bae7..ebe193ba3871 100644
@@ -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 {

drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c +4 -1

index 339b5d31e1f1..5c84dd63778e 100644
@@ -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(

drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c +2 -0

index 70b53988553c..f3e18d627b0a 100644
@@ -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 },
 	{}

Signature · 1 line

2.15.1

Andy Shevchenko

author

2017-12-19 21:00 UTC #4 lore raw

PATCH 5/10 staging: atomisp: Remove non-ACPI leftovers

In reply to 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 <[email protected]>

---
 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(-)

drivers/staging/media/atomisp/i2c/atomisp-gc0310.c +3 -7

index e70d8afcc229..61b7598469eb 100644
@@ -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;

drivers/staging/media/atomisp/i2c/atomisp-gc2235.c +2 -6

index 85da5fe24033..d8de46da64ae 100644
@@ -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);

drivers/staging/media/atomisp/i2c/atomisp-lm3554.c +8 ~~-20~~

index 974b6ff50c7a..7098bf317f16 100644
@@ -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);

drivers/staging/media/atomisp/i2c/atomisp-mt9m114.c +3 -5

index 55882bea2049..df253a557c76 100644
@@ -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) {

drivers/staging/media/atomisp/i2c/atomisp-ov2680.c +3 -7

index cd67d38f183a..84f8d33ce2d1 100644
@@ -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;

drivers/staging/media/atomisp/i2c/atomisp-ov2722.c +4 ~~-13~~

index 4df7eba8d375..2b6ae0faf972 100644
@@ -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);

drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c +3 -9

index 3e7c3851280f..2a680eae20d3 100644
@@ -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;

drivers/staging/media/atomisp/i2c/ov8858.c +21 ~~-22~~

50 lines not on this page · raw message

drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c +2 -4

index 8fb5147531a5..8dcec0e780a1 100644
@@ -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)

Signature · 1 line

2.15.1

Dan Carpenter

2017-12-20 04:54 UTC #5 lore raw

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

drivers/staging/media/atomisp/i2c/atomisp-gc2235.c static int gc2235_probe(struct i2c_client *client) · line 1146

Quoted 3 lines · Andy Shevchenko

drivers/staging/media/atomisp/i2c/atomisp-gc2235.c +0 -0

@@ -1147,10 +1145,8 @@ static int gc2235_probe(struct i2c_client *client)
 	if (ret)
 		gc2235_remove(client);

This error handling is probably wrong...

drivers/staging/media/atomisp/i2c/atomisp-gc2235.c static int gc2235_probe(struct i2c_client *client) · line 1148

Quoted 4 lines

drivers/staging/media/atomisp/i2c/atomisp-gc2235.c +1 -2

-	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;

drivers/staging/media/atomisp/i2c/atomisp-gc2235.c static int gc2235_probe(struct i2c_client *client) · line 1152

Quoted 5 lines

drivers/staging/media/atomisp/i2c/atomisp-gc2235.c +0 -1
-	return ret;
 out_free:
 	v4l2_device_unregister_subdev(&dev->sd);
 	kfree(dev);

regards,
dan carpenter

Andy Shevchenko

2017-12-20 10:24 UTC #6 lore raw

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

Quoted 7 lines · Dan Carpenter

drivers/staging/media/atomisp/i2c/atomisp-gc2235.c static int gc2235_probe(struct i2c_client *client) · line 1146
Quoted 3 lines · Andy Shevchenko
drivers/staging/media/atomisp/i2c/atomisp-gc2235.c +0 -0
@@ -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.

Quoted 22 lines

drivers/staging/media/atomisp/i2c/atomisp-gc2235.c static int gc2235_probe(struct i2c_client *client) · line 1148
Quoted 4 lines
drivers/staging/media/atomisp/i2c/atomisp-gc2235.c +1 -2
-     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;
drivers/staging/media/atomisp/i2c/atomisp-gc2235.c static int gc2235_probe(struct i2c_client *client) · line 1152
Quoted 5 lines
drivers/staging/media/atomisp/i2c/atomisp-gc2235.c +0 -1
-     return ret;
 out_free:
      v4l2_device_unregister_subdev(&dev->sd);
      kfree(dev);
regards,
dan carpenter

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Sakari Ailus

reviewer

2017-12-21 22:34 UTC #7 lore raw

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

Hi Andy and Dan,

Quoted 9 lines · Andy Shevchenko

Quoted 7 lines · Dan Carpenter
drivers/staging/media/atomisp/i2c/atomisp-gc2235.c static int gc2235_probe(struct i2c_client *client) · line 1146
Quoted 3 lines · Andy Shevchenko
drivers/staging/media/atomisp/i2c/atomisp-gc2235.c +0 -0
@@ -1147,10 +1145,8 @@ static int gc2235_probe(struct i2c_client *client)
      if (ret)
              gc2235_remove(client);
This error handling is probably wrong...

Quoted 8 lines

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!

Signature · 4 lines

Kind regards,

Sakari Ailus
[email protected]

Dan Carpenter

2017-12-20 05:38 UTC #8 lore raw

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

In reply to Andy Shevchenko

drivers/staging/media/atomisp/i2c/atomisp-lm3554.c static int lm3554_probe(struct i2c_client *client) · line 910

Quoted 11 lines · Andy Shevchenko

drivers/staging/media/atomisp/i2c/atomisp-lm3554.c +1 -3

@@ -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

Julia Lawall

2017-12-20 10:30 UTC #9 lore raw

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

Quoted 18 lines · Dan Carpenter

drivers/staging/media/atomisp/i2c/atomisp-lm3554.c static int lm3554_probe(struct i2c_client *client) · line 910
Quoted 11 lines · Andy Shevchenko
drivers/staging/media/atomisp/i2c/atomisp-lm3554.c +1 -3
@@ -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.

Quoted 1 line

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

Dan Carpenter

2018-01-02 10:26 UTC #10 lore raw

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

Quoted 22 lines · Julia Lawall

Quoted 18 lines · Dan Carpenter
drivers/staging/media/atomisp/i2c/atomisp-lm3554.c static int lm3554_probe(struct i2c_client *client) · line 910
Quoted 11 lines · Andy Shevchenko
drivers/staging/media/atomisp/i2c/atomisp-lm3554.c +1 -3
@@ -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.

Quoted 9 lines

Quoted 1 line
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

Julia Lawall

2018-01-02 10:36 UTC #11 lore raw

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

Quoted 44 lines · Dan Carpenter

Quoted 22 lines · Julia Lawall
Quoted 18 lines · Dan Carpenter
drivers/staging/media/atomisp/i2c/atomisp-lm3554.c static int lm3554_probe(struct i2c_client *client) · line 910
Quoted 11 lines · Andy Shevchenko
drivers/staging/media/atomisp/i2c/atomisp-lm3554.c +1 -3
@@ -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.
Quoted 9 lines
Quoted 1 line
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();

Quoted 1 line

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

walter harms

2017-12-20 12:27 UTC #12 lore raw

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

In reply to Dan Carpenter

Am 20.12.2017 11:30, schrieb Julia Lawall:

Quoted 22 lines · Dan Carpenter

Quoted 19 lines · Andy Shevchenko
drivers/staging/media/atomisp/i2c/atomisp-lm3554.c static int lm3554_probe(struct i2c_client *client) · line 910
Quoted 11 lines
drivers/staging/media/atomisp/i2c/atomisp-lm3554.c +1 -3
@@ -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.

Quoted 9 lines

Quoted 1 line
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

Julia Lawall

2017-12-20 12:36 UTC #13 lore raw

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

In reply to Dan Carpenter

Quoted 35 lines · walter harms

Am 20.12.2017 11:30, schrieb Julia Lawall:
Quoted 22 lines · Dan Carpenter
Quoted 19 lines · Andy Shevchenko
drivers/staging/media/atomisp/i2c/atomisp-lm3554.c static int lm3554_probe(struct i2c_client *client) · line 910
Quoted 11 lines
drivers/staging/media/atomisp/i2c/atomisp-lm3554.c +1 -3
@@ -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.
Quoted 9 lines
Quoted 1 line
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?

Quoted 4 lines

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

Quoted 8 lines

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

Andy Shevchenko

author

2017-12-19 21:00 UTC #14 lore raw

PATCH 6/10 staging: atomisp: Switch to use struct device_driver directly

In reply to 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 <[email protected]>

---
 .../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(-)

drivers/staging/media/atomisp/pci/atomisp2/atomisp_drvfs.c +8 -9

index 7129b88456cb..ceedb82b6beb 100644
@@ -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;

drivers/staging/media/atomisp/pci/atomisp2/atomisp_drvfs.h +2 -3

index b91bfef21639..7c99240d107a 100644
@@ -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__ */

drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c +1 -3

index 3c260f8b52e2..7a9efc6847ca 100644
@@ -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;

Signature · 1 line

2.15.1

Andy Shevchenko

author

2017-12-19 21:08 UTC #15 lore raw

PATCH 7/10 staging: atomisp: Remove redundant PCI code

In reply to Andy Shevchenko

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

Signed-off-by Andy Shevchenko <[email protected]>

---
 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(-)

drivers/staging/media/atomisp/pci/atomisp2/atomisp_internal.h +0 -1

index 52a6f8002048..dc476a3dd271 100644
@@ -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;

drivers/staging/media/atomisp/pci/atomisp2/atomisp_v4l2.c +0 -8

index 7a9efc6847ca..548e00e7d67b 100644
@@ -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);

Signature · 1 line

2.15.1

Andy Shevchenko

author

2017-12-19 21:01 UTC #16 lore raw

PATCH 8/10 staging: atomisp: Unexport local function

In reply to 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 <[email protected]>

---
 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(-)

drivers/staging/media/atomisp/include/linux/atomisp_gmin_platform.h +0 -1

index 7e3ca12dd4e9..c52c56a17e17 100644
@@ -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);

drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c +2 -3

index 8dcec0e780a1..0f859bb714bf 100644
@@ -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)
 {

Signature · 1 line

2.15.1

Andy Shevchenko

author

2017-12-19 21:01 UTC #17 lore raw

PATCH 9/10 staging: atomisp: Use standard DMI match table

In reply to 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 <[email protected]>

---
 .../platform/intel-mid/atomisp_gmin_platform.c     | 109 +++++++++++++--------
 1 file changed, 70 insertions(+), 39 deletions(-)

drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c +70 ~~-39~~

index 0f859bb714bf..8408a58ed764 100644
@@ -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.

Signature · 1 line

2.15.1

Andy Shevchenko

author

2017-12-19 21:00 UTC #18 lore raw

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

In reply to 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 <[email protected]>

---
 .../staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c   | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/staging/media/atomisp/platform/intel-mid/atomisp_gmin_platform.c +2 -1

index 8408a58ed764..d8b7183db252 100644
@@ -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,
 	},

Signature · 1 line

2.15.1