[PATCH v3 0/6] media: video-i2c: support changing frame interval and runtime PM

[PATCH v3 0/6] media: video-i2c: support changing frame interval and runtime PM

[Akinobu Mita]

This patchset adds support for changing frame interval and runtime PM for
video-i2c driver.  This also adds an helper macro to v4l2 common
internal API that is used to to find a suitable frame interval.

There are a couple of unrelated changes that are included for simplifying
driver initialization code and register accesses.

* v3
- Append result of v4l2-compliance in cover-letter
- Move the code causing use-after-free from video_i2c_remove() to the
  video device release() callback.
- Use regmap_init_i2c() instead of devm_regmap_init_i2c() and call
  regmap_exit_i2c() in video device release() callback in order to
  avoid releasing regmap when the driver is unbound.
- Add Acked-by lines

* v2
- Add Acked-by and Reviewed-by tags
- Update commit log to clarify the use after free
- Add thermistor and termperature register address definisions
- Stop adding v4l2_find_closest_fract() in v4l2 common internal API
- Add V4L2_FRACT_COMPARE() macro in v4l2 common internal API
- Use V4L2_FRACT_COMPARE() to find suitable frame interval instead of
  v4l2_find_closest_fract()
- Add comment for register address definisions

Akinobu Mita (6):
  media: video-i2c: avoid accessing released memory area when removing
    driver
  media: video-i2c: use i2c regmap
  media: v4l2-common: add V4L2_FRACT_COMPARE
  media: vivid: use V4L2_FRACT_COMPARE
  media: video-i2c: support changing frame interval
  media: video-i2c: support runtime PM

 drivers/media/i2c/video-i2c.c                | 286 +++++++++++++++++++++++----
 drivers/media/platform/vivid/vivid-vid-cap.c |   9 +-
 include/media/v4l2-common.h                  |   5 +
 3 files changed, 254 insertions(+), 46 deletions(-)

v4l2-compliance SHA: 7bde5ef172bd4a09b9544788ba9c5dbb1aa9994a, 32 bits

Compliance test for device /dev/video2:

Driver Info:
	Driver name      : video-i2c
	Card type        : I2C 1-104 Transport Video
	Bus info         : I2C:1-104
	Driver version   : 4.19.0
	Capabilities     : 0x85200001
		Video Capture
		Read/Write
		Streaming
		Extended Pix Format
		Device Capabilities
	Device Caps      : 0x05200001
		Video Capture
		Read/Write
		Streaming
		Extended Pix Format

Required ioctls:
	test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
	test second /dev/video2 open: OK
	test VIDIOC_QUERYCAP: OK
	test VIDIOC_G/S_PRIORITY: OK
	test for unlimited opens: OK

Debug ioctls:
	test VIDIOC_DBG_G/S_REGISTER: OK
	test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
	test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
	test VIDIOC_ENUMAUDIO: OK (Not Supported)
	test VIDIOC_G/S/ENUMINPUT: OK
	test VIDIOC_G/S_AUDIO: OK (Not Supported)
	Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
	test VIDIOC_G/S_MODULATOR: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_ENUMAUDOUT: OK (Not Supported)
	test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
	test VIDIOC_G/S_AUDOUT: OK (Not Supported)
	Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
	test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
	test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
	test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
	test VIDIOC_G/S_EDID: OK (Not Supported)

Control ioctls (Input 0):
	test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
	test VIDIOC_QUERYCTRL: OK (Not Supported)
	test VIDIOC_G/S_CTRL: OK (Not Supported)
	test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
	test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
	test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
	Standard Controls: 0 Private Controls: 0

Format ioctls (Input 0):
	test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
	test VIDIOC_G/S_PARM: OK
	test VIDIOC_G_FBUF: OK (Not Supported)
	test VIDIOC_G_FMT: OK
	test VIDIOC_TRY_FMT: OK
	test VIDIOC_S_FMT: OK
	test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
	test Cropping: OK (Not Supported)
	test Composing: OK (Not Supported)
	test Scaling: OK (Not Supported)

Codec ioctls (Input 0):
	test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
	test VIDIOC_G_ENC_INDEX: OK (Not Supported)
	test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls (Input 0):
	test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
	test VIDIOC_EXPBUF: OK (Not Supported)

Total: 43, Succeeded: 43, Failed: 0, Warnings: 0

Cc: Matt Ranostay <matt.ranostay@konsulko.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Hans Verkuil <hansverk@cisco.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
-- 
2.7.4

[PATCH v3 1/6] media: video-i2c: avoid accessing released memory area when removing driver

[Akinobu Mita]

The video device release() callback for video-i2c driver frees the whole
struct video_i2c_data.  If there is no user left for the video device
when video_unregister_device() is called, the release callback is executed.

However, in video_i2c_remove() some fields (v4l2_dev, lock, and queue_lock)
in struct video_i2c_data are still accessed after video_unregister_device()
is called.

This fixes the use after free by moving the code from video_i2c_remove()
to the release() callback.

Fixes: 5cebaac60974 ("media: video-i2c: add video-i2c driver")
Cc: Matt Ranostay <matt.ranostay@konsulko.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Hans Verkuil <hansverk@cisco.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v3
- Move the code causing use-after-free from video_i2c_remove() to the
  video device release() callback.
- Remove Acked-by line as there are enough changes from previous version

 drivers/media/i2c/video-i2c.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index 06d29d8..f27d294 100644
--- a/drivers/media/i2c/video-i2c.c
+++ b/drivers/media/i2c/video-i2c.c
@@ -510,7 +510,12 @@ static const struct v4l2_ioctl_ops video_i2c_ioctl_ops = {
 
 static void video_i2c_release(struct video_device *vdev)
 {
-	kfree(video_get_drvdata(vdev));
+	struct video_i2c_data *data = video_get_drvdata(vdev);
+
+	v4l2_device_unregister(&data->v4l2_dev);
+	mutex_destroy(&data->lock);
+	mutex_destroy(&data->queue_lock);
+	kfree(data);
 }
 
 static int video_i2c_probe(struct i2c_client *client,
@@ -608,10 +613,6 @@ static int video_i2c_remove(struct i2c_client *client)
 	struct video_i2c_data *data = i2c_get_clientdata(client);
 
 	video_unregister_device(&data->vdev);
-	v4l2_device_unregister(&data->v4l2_dev);
-
-	mutex_destroy(&data->lock);
-	mutex_destroy(&data->queue_lock);
 
 	return 0;
 }
-- 
2.7.4

[PATCH v3 2/6] media: video-i2c: use i2c regmap

[Akinobu Mita]

Use regmap for i2c register access.  This simplifies register accesses and
chooses suitable access commands based on the functionality that the
adapter supports.

Cc: Matt Ranostay <matt.ranostay@konsulko.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Hans Verkuil <hansverk@cisco.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Acked-by: Matt Ranostay <matt.ranostay@konsulko.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v3
- Use regmap_init_i2c() instead of devm_regmap_init_i2c() and call
  regmap_exit_i2c() in video device release() callback in order to
  avoid releasing regmap when the driver is unbound.
- Add Acked-by lines

 drivers/media/i2c/video-i2c.c | 68 ++++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 27 deletions(-)

diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index f27d294..f23cb91 100644
--- a/drivers/media/i2c/video-i2c.c
+++ b/drivers/media/i2c/video-i2c.c
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of_device.h>
+#include <linux/regmap.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/videodev2.h>
@@ -38,7 +39,7 @@ struct video_i2c_buffer {
 };
 
 struct video_i2c_data {
-	struct i2c_client *client;
+	struct regmap *regmap;
 	const struct video_i2c_chip *chip;
 	struct mutex lock;
 	spinlock_t slock;
@@ -62,6 +63,12 @@ static const struct v4l2_frmsize_discrete amg88xx_size = {
 	.height = 8,
 };
 
+static const struct regmap_config amg88xx_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0xff
+};
+
 struct video_i2c_chip {
 	/* video dimensions */
 	const struct v4l2_fmtdesc *format;
@@ -76,6 +83,8 @@ struct video_i2c_chip {
 	/* pixel size in bits */
 	unsigned int bpp;
 
+	const struct regmap_config *regmap_config;
+
 	/* xfer function */
 	int (*xfer)(struct video_i2c_data *data, char *buf);
 
@@ -83,26 +92,16 @@ struct video_i2c_chip {
 	int (*hwmon_init)(struct video_i2c_data *data);
 };
 
-static int amg88xx_xfer(struct video_i2c_data *data, char *buf)
-{
-	struct i2c_client *client = data->client;
-	struct i2c_msg msg[2];
-	u8 reg = 0x80;
-	int ret;
-
-	msg[0].addr = client->addr;
-	msg[0].flags = 0;
-	msg[0].len = 1;
-	msg[0].buf  = (char *)&reg;
+/* Thermistor register */
+#define AMG88XX_REG_TTHL	0x0e
 
-	msg[1].addr = client->addr;
-	msg[1].flags = I2C_M_RD;
-	msg[1].len = data->chip->buffer_size;
-	msg[1].buf = (char *)buf;
+/* Temperature register */
+#define AMG88XX_REG_T01L	0x80
 
-	ret = i2c_transfer(client->adapter, msg, 2);
-
-	return (ret == 2) ? 0 : -EIO;
+static int amg88xx_xfer(struct video_i2c_data *data, char *buf)
+{
+	return regmap_bulk_read(data->regmap, AMG88XX_REG_T01L, buf,
+				data->chip->buffer_size);
 }
 
 #if IS_ENABLED(CONFIG_HWMON)
@@ -133,12 +132,15 @@ static int amg88xx_read(struct device *dev, enum hwmon_sensor_types type,
 			u32 attr, int channel, long *val)
 {
 	struct video_i2c_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	int tmp = i2c_smbus_read_word_data(client, 0x0e);
+	__le16 buf;
+	int tmp;
 
-	if (tmp < 0)
+	tmp = regmap_bulk_read(data->regmap, AMG88XX_REG_TTHL, &buf, 2);
+	if (tmp)
 		return tmp;
 
+	tmp = le16_to_cpu(buf);
+
 	/*
 	 * Check for sign bit, this isn't a two's complement value but an
 	 * absolute temperature that needs to be inverted in the case of being
@@ -164,8 +166,9 @@ static const struct hwmon_chip_info amg88xx_chip_info = {
 
 static int amg88xx_hwmon_init(struct video_i2c_data *data)
 {
-	void *hwmon = devm_hwmon_device_register_with_info(&data->client->dev,
-				"amg88xx", data, &amg88xx_chip_info, NULL);
+	struct device *dev = regmap_get_device(data->regmap);
+	void *hwmon = devm_hwmon_device_register_with_info(dev, "amg88xx", data,
+						&amg88xx_chip_info, NULL);
 
 	return PTR_ERR_OR_ZERO(hwmon);
 }
@@ -182,6 +185,7 @@ static const struct video_i2c_chip video_i2c_chip[] = {
 		.max_fps	= 10,
 		.buffer_size	= 128,
 		.bpp		= 16,
+		.regmap_config	= &amg88xx_regmap_config,
 		.xfer		= &amg88xx_xfer,
 		.hwmon_init	= amg88xx_hwmon_init,
 	},
@@ -350,7 +354,8 @@ static int video_i2c_querycap(struct file *file, void  *priv,
 				struct v4l2_capability *vcap)
 {
 	struct video_i2c_data *data = video_drvdata(file);
-	struct i2c_client *client = data->client;
+	struct device *dev = regmap_get_device(data->regmap);
+	struct i2c_client *client = to_i2c_client(dev);
 
 	strlcpy(vcap->driver, data->v4l2_dev.name, sizeof(vcap->driver));
 	strlcpy(vcap->card, data->vdev.name, sizeof(vcap->card));
@@ -515,6 +520,7 @@ static void video_i2c_release(struct video_device *vdev)
 	v4l2_device_unregister(&data->v4l2_dev);
 	mutex_destroy(&data->lock);
 	mutex_destroy(&data->queue_lock);
+	regmap_exit(data->regmap);
 	kfree(data);
 }
 
@@ -537,13 +543,18 @@ static int video_i2c_probe(struct i2c_client *client,
 	else
 		goto error_free_device;
 
-	data->client = client;
+	data->regmap = regmap_init_i2c(client, data->chip->regmap_config);
+	if (IS_ERR(data->regmap)) {
+		ret = PTR_ERR(data->regmap);
+		goto error_free_device;
+	}
+
 	v4l2_dev = &data->v4l2_dev;
 	strlcpy(v4l2_dev->name, VIDEO_I2C_DRIVER, sizeof(v4l2_dev->name));
 
 	ret = v4l2_device_register(&client->dev, v4l2_dev);
 	if (ret < 0)
-		goto error_free_device;
+		goto error_regmap_exit;
 
 	mutex_init(&data->lock);
 	mutex_init(&data->queue_lock);
@@ -602,6 +613,9 @@ static int video_i2c_probe(struct i2c_client *client,
 	mutex_destroy(&data->lock);
 	mutex_destroy(&data->queue_lock);
 
+error_regmap_exit:
+	regmap_exit(data->regmap);
+
 error_free_device:
 	kfree(data);
 
-- 
2.7.4

[PATCH v3 3/6] media: v4l2-common: add V4L2_FRACT_COMPARE

[Akinobu Mita]

Add macro to compare two v4l2_fract values in v4l2 common internal API.
The same macro FRACT_CMP() is used by vivid and bcm2835-camera.  This just
renames it to V4L2_FRACT_COMPARE in order to avoid namespace collision.

Cc: Matt Ranostay <matt.ranostay@konsulko.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Hans Verkuil <hansverk@cisco.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v3
- Add Acked-by line

 include/media/v4l2-common.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index cdc87ec..eafb8a3 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -384,4 +384,9 @@ int v4l2_g_parm_cap(struct video_device *vdev,
 int v4l2_s_parm_cap(struct video_device *vdev,
 		    struct v4l2_subdev *sd, struct v4l2_streamparm *a);
 
+/* Compare two v4l2_fract structs */
+#define V4L2_FRACT_COMPARE(a, OP, b)			\
+	((u64)(a).numerator * (b).denominator OP	\
+	(u64)(b).numerator * (a).denominator)
+
 #endif /* V4L2_COMMON_H_ */
-- 
2.7.4

[PATCH v3 4/6] media: vivid: use V4L2_FRACT_COMPARE

[Akinobu Mita]

Now the equivalent of FRACT_CMP() is added in v4l2 common internal API
header.

Cc: Matt Ranostay <matt.ranostay@konsulko.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Hans Verkuil <hansverk@cisco.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v3
- Add Acked-by line

 drivers/media/platform/vivid/vivid-vid-cap.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/vivid/vivid-vid-cap.c b/drivers/media/platform/vivid/vivid-vid-cap.c
index 1599159..f19c701 100644
--- a/drivers/media/platform/vivid/vivid-vid-cap.c
+++ b/drivers/media/platform/vivid/vivid-vid-cap.c
@@ -1824,9 +1824,6 @@ int vivid_vid_cap_g_parm(struct file *file, void *priv,
 	return 0;
 }
 
-#define FRACT_CMP(a, OP, b)	\
-	((u64)(a).numerator * (b).denominator  OP  (u64)(b).numerator * (a).denominator)
-
 int vivid_vid_cap_s_parm(struct file *file, void *priv,
 			  struct v4l2_streamparm *parm)
 {
@@ -1847,14 +1844,14 @@ int vivid_vid_cap_s_parm(struct file *file, void *priv,
 	if (tpf.denominator == 0)
 		tpf = webcam_intervals[ival_sz - 1];
 	for (i = 0; i < ival_sz; i++)
-		if (FRACT_CMP(tpf, >=, webcam_intervals[i]))
+		if (V4L2_FRACT_COMPARE(tpf, >=, webcam_intervals[i]))
 			break;
 	if (i == ival_sz)
 		i = ival_sz - 1;
 	dev->webcam_ival_idx = i;
 	tpf = webcam_intervals[dev->webcam_ival_idx];
-	tpf = FRACT_CMP(tpf, <, tpf_min) ? tpf_min : tpf;
-	tpf = FRACT_CMP(tpf, >, tpf_max) ? tpf_max : tpf;
+	tpf = V4L2_FRACT_COMPARE(tpf, <, tpf_min) ? tpf_min : tpf;
+	tpf = V4L2_FRACT_COMPARE(tpf, >, tpf_max) ? tpf_max : tpf;
 
 	/* resync the thread's timings */
 	dev->cap_seq_resync = true;
-- 
2.7.4

[PATCH v3 5/6] media: video-i2c: support changing frame interval

[Akinobu Mita]

AMG88xx has a register for setting frame rate 1 or 10 FPS.
This adds support changing frame interval.

Reference specifications:
https://docid81hrs3j1.cloudfront.net/medialibrary/2017/11/PANA-S-A0002141979-1.pdf

Cc: Matt Ranostay <matt.ranostay@konsulko.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Hans Verkuil <hansverk@cisco.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Acked-by: Matt Ranostay <matt.ranostay@konsulko.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v3
- Add Acked-by line

 drivers/media/i2c/video-i2c.c | 78 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 66 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index f23cb91..3334fc2 100644
--- a/drivers/media/i2c/video-i2c.c
+++ b/drivers/media/i2c/video-i2c.c
@@ -52,6 +52,8 @@ struct video_i2c_data {
 
 	struct task_struct *kthread_vid_cap;
 	struct list_head vid_cap_active;
+
+	struct v4l2_fract frame_interval;
 };
 
 static const struct v4l2_fmtdesc amg88xx_format = {
@@ -74,8 +76,9 @@ struct video_i2c_chip {
 	const struct v4l2_fmtdesc *format;
 	const struct v4l2_frmsize_discrete *size;
 
-	/* max frames per second */
-	unsigned int max_fps;
+	/* available frame intervals */
+	const struct v4l2_fract *frame_intervals;
+	unsigned int num_frame_intervals;
 
 	/* pixel buffer size */
 	unsigned int buffer_size;
@@ -85,6 +88,9 @@ struct video_i2c_chip {
 
 	const struct regmap_config *regmap_config;
 
+	/* setup function */
+	int (*setup)(struct video_i2c_data *data);
+
 	/* xfer function */
 	int (*xfer)(struct video_i2c_data *data, char *buf);
 
@@ -92,6 +98,10 @@ struct video_i2c_chip {
 	int (*hwmon_init)(struct video_i2c_data *data);
 };
 
+/* Frame rate register */
+#define AMG88XX_REG_FPSC	0x02
+#define AMG88XX_FPSC_1FPS		BIT(0)
+
 /* Thermistor register */
 #define AMG88XX_REG_TTHL	0x0e
 
@@ -104,6 +114,19 @@ static int amg88xx_xfer(struct video_i2c_data *data, char *buf)
 				data->chip->buffer_size);
 }
 
+static int amg88xx_setup(struct video_i2c_data *data)
+{
+	unsigned int mask = AMG88XX_FPSC_1FPS;
+	unsigned int val;
+
+	if (data->frame_interval.numerator == data->frame_interval.denominator)
+		val = mask;
+	else
+		val = 0;
+
+	return regmap_update_bits(data->regmap, AMG88XX_REG_FPSC, mask, val);
+}
+
 #if IS_ENABLED(CONFIG_HWMON)
 
 static const u32 amg88xx_temp_config[] = {
@@ -178,14 +201,21 @@ static int amg88xx_hwmon_init(struct video_i2c_data *data)
 
 #define AMG88XX		0
 
+static const struct v4l2_fract amg88xx_frame_intervals[] = {
+	{ 1, 10 },
+	{ 1, 1 },
+};
+
 static const struct video_i2c_chip video_i2c_chip[] = {
 	[AMG88XX] = {
 		.size		= &amg88xx_size,
 		.format		= &amg88xx_format,
-		.max_fps	= 10,
+		.frame_intervals	= amg88xx_frame_intervals,
+		.num_frame_intervals	= ARRAY_SIZE(amg88xx_frame_intervals),
 		.buffer_size	= 128,
 		.bpp		= 16,
 		.regmap_config	= &amg88xx_regmap_config,
+		.setup		= &amg88xx_setup,
 		.xfer		= &amg88xx_xfer,
 		.hwmon_init	= amg88xx_hwmon_init,
 	},
@@ -250,7 +280,8 @@ static void buffer_queue(struct vb2_buffer *vb)
 static int video_i2c_thread_vid_cap(void *priv)
 {
 	struct video_i2c_data *data = priv;
-	unsigned int delay = msecs_to_jiffies(1000 / data->chip->max_fps);
+	unsigned int delay = mult_frac(HZ, data->frame_interval.numerator,
+				       data->frame_interval.denominator);
 
 	set_freezable();
 
@@ -312,19 +343,26 @@ static void video_i2c_del_list(struct vb2_queue *vq, enum vb2_buffer_state state
 static int start_streaming(struct vb2_queue *vq, unsigned int count)
 {
 	struct video_i2c_data *data = vb2_get_drv_priv(vq);
+	int ret;
 
 	if (data->kthread_vid_cap)
 		return 0;
 
+	ret = data->chip->setup(data);
+	if (ret)
+		goto error_del_list;
+
 	data->sequence = 0;
 	data->kthread_vid_cap = kthread_run(video_i2c_thread_vid_cap, data,
 					    "%s-vid-cap", data->v4l2_dev.name);
-	if (!IS_ERR(data->kthread_vid_cap))
+	ret = PTR_ERR_OR_ZERO(data->kthread_vid_cap);
+	if (!ret)
 		return 0;
 
+error_del_list:
 	video_i2c_del_list(vq, VB2_BUF_STATE_QUEUED);
 
-	return PTR_ERR(data->kthread_vid_cap);
+	return ret;
 }
 
 static void stop_streaming(struct vb2_queue *vq)
@@ -431,15 +469,14 @@ static int video_i2c_enum_frameintervals(struct file *file, void *priv,
 	const struct video_i2c_data *data = video_drvdata(file);
 	const struct v4l2_frmsize_discrete *size = data->chip->size;
 
-	if (fe->index > 0)
+	if (fe->index >= data->chip->num_frame_intervals)
 		return -EINVAL;
 
 	if (fe->width != size->width || fe->height != size->height)
 		return -EINVAL;
 
 	fe->type = V4L2_FRMIVAL_TYPE_DISCRETE;
-	fe->discrete.numerator = 1;
-	fe->discrete.denominator = data->chip->max_fps;
+	fe->discrete = data->chip->frame_intervals[fe->index];
 
 	return 0;
 }
@@ -484,12 +521,27 @@ static int video_i2c_g_parm(struct file *filp, void *priv,
 
 	parm->parm.capture.readbuffers = 1;
 	parm->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
-	parm->parm.capture.timeperframe.numerator = 1;
-	parm->parm.capture.timeperframe.denominator = data->chip->max_fps;
+	parm->parm.capture.timeperframe = data->frame_interval;
 
 	return 0;
 }
 
+static int video_i2c_s_parm(struct file *filp, void *priv,
+			      struct v4l2_streamparm *parm)
+{
+	struct video_i2c_data *data = video_drvdata(filp);
+	int i;
+
+	for (i = 0; i < data->chip->num_frame_intervals - 1; i++) {
+		if (V4L2_FRACT_COMPARE(parm->parm.capture.timeperframe, <=,
+				       data->chip->frame_intervals[i]))
+			break;
+	}
+	data->frame_interval = data->chip->frame_intervals[i];
+
+	return video_i2c_g_parm(filp, priv, parm);
+}
+
 static const struct v4l2_ioctl_ops video_i2c_ioctl_ops = {
 	.vidioc_querycap		= video_i2c_querycap,
 	.vidioc_g_input			= video_i2c_g_input,
@@ -501,7 +553,7 @@ static const struct v4l2_ioctl_ops video_i2c_ioctl_ops = {
 	.vidioc_g_fmt_vid_cap		= video_i2c_try_fmt_vid_cap,
 	.vidioc_s_fmt_vid_cap		= video_i2c_s_fmt_vid_cap,
 	.vidioc_g_parm			= video_i2c_g_parm,
-	.vidioc_s_parm			= video_i2c_g_parm,
+	.vidioc_s_parm			= video_i2c_s_parm,
 	.vidioc_try_fmt_vid_cap		= video_i2c_try_fmt_vid_cap,
 	.vidioc_reqbufs			= vb2_ioctl_reqbufs,
 	.vidioc_create_bufs		= vb2_ioctl_create_bufs,
@@ -591,6 +643,8 @@ static int video_i2c_probe(struct i2c_client *client,
 	spin_lock_init(&data->slock);
 	INIT_LIST_HEAD(&data->vid_cap_active);
 
+	data->frame_interval = data->chip->frame_intervals[0];
+
 	video_set_drvdata(&data->vdev, data);
 	i2c_set_clientdata(client, data);
 
-- 
2.7.4

[PATCH v3 6/6] media: video-i2c: support runtime PM

[Akinobu Mita]

AMG88xx has a register for setting operating mode.  This adds support
runtime PM by changing the operating mode.

The instruction for changing sleep mode to normal mode is from the
reference specifications.

https://docid81hrs3j1.cloudfront.net/medialibrary/2017/11/PANA-S-A0002141979-1.pdf

Cc: Matt Ranostay <matt.ranostay@konsulko.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Hans Verkuil <hansverk@cisco.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Reviewed-by: Matt Ranostay <matt.ranostay@konsulko.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v3
- Move chip->set_power() call to the video device release() callback.
- Add Acked-by line

 drivers/media/i2c/video-i2c.c | 141 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 139 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index 3334fc2..22fdc43 100644
--- a/drivers/media/i2c/video-i2c.c
+++ b/drivers/media/i2c/video-i2c.c
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
@@ -94,10 +95,23 @@ struct video_i2c_chip {
 	/* xfer function */
 	int (*xfer)(struct video_i2c_data *data, char *buf);
 
+	/* power control function */
+	int (*set_power)(struct video_i2c_data *data, bool on);
+
 	/* hwmon init function */
 	int (*hwmon_init)(struct video_i2c_data *data);
 };
 
+/* Power control register */
+#define AMG88XX_REG_PCTL	0x00
+#define AMG88XX_PCTL_NORMAL		0x00
+#define AMG88XX_PCTL_SLEEP		0x10
+
+/* Reset register */
+#define AMG88XX_REG_RST		0x01
+#define AMG88XX_RST_FLAG		0x30
+#define AMG88XX_RST_INIT		0x3f
+
 /* Frame rate register */
 #define AMG88XX_REG_FPSC	0x02
 #define AMG88XX_FPSC_1FPS		BIT(0)
@@ -127,6 +141,59 @@ static int amg88xx_setup(struct video_i2c_data *data)
 	return regmap_update_bits(data->regmap, AMG88XX_REG_FPSC, mask, val);
 }
 
+static int amg88xx_set_power_on(struct video_i2c_data *data)
+{
+	int ret;
+
+	ret = regmap_write(data->regmap, AMG88XX_REG_PCTL, AMG88XX_PCTL_NORMAL);
+	if (ret)
+		return ret;
+
+	msleep(50);
+
+	ret = regmap_write(data->regmap, AMG88XX_REG_RST, AMG88XX_RST_INIT);
+	if (ret)
+		return ret;
+
+	msleep(2);
+
+	ret = regmap_write(data->regmap, AMG88XX_REG_RST, AMG88XX_RST_FLAG);
+	if (ret)
+		return ret;
+
+	/*
+	 * Wait two frames before reading thermistor and temperature registers
+	 */
+	msleep(200);
+
+	return 0;
+}
+
+static int amg88xx_set_power_off(struct video_i2c_data *data)
+{
+	int ret;
+
+	ret = regmap_write(data->regmap, AMG88XX_REG_PCTL, AMG88XX_PCTL_SLEEP);
+	if (ret)
+		return ret;
+	/*
+	 * Wait for a while to avoid resuming normal mode immediately after
+	 * entering sleep mode, otherwise the device occasionally goes wrong
+	 * (thermistor and temperature registers are not updated at all)
+	 */
+	msleep(100);
+
+	return 0;
+}
+
+static int amg88xx_set_power(struct video_i2c_data *data, bool on)
+{
+	if (on)
+		return amg88xx_set_power_on(data);
+
+	return amg88xx_set_power_off(data);
+}
+
 #if IS_ENABLED(CONFIG_HWMON)
 
 static const u32 amg88xx_temp_config[] = {
@@ -158,7 +225,15 @@ static int amg88xx_read(struct device *dev, enum hwmon_sensor_types type,
 	__le16 buf;
 	int tmp;
 
+	tmp = pm_runtime_get_sync(regmap_get_device(data->regmap));
+	if (tmp < 0) {
+		pm_runtime_put_noidle(regmap_get_device(data->regmap));
+		return tmp;
+	}
+
 	tmp = regmap_bulk_read(data->regmap, AMG88XX_REG_TTHL, &buf, 2);
+	pm_runtime_mark_last_busy(regmap_get_device(data->regmap));
+	pm_runtime_put_autosuspend(regmap_get_device(data->regmap));
 	if (tmp)
 		return tmp;
 
@@ -217,6 +292,7 @@ static const struct video_i2c_chip video_i2c_chip[] = {
 		.regmap_config	= &amg88xx_regmap_config,
 		.setup		= &amg88xx_setup,
 		.xfer		= &amg88xx_xfer,
+		.set_power	= amg88xx_set_power,
 		.hwmon_init	= amg88xx_hwmon_init,
 	},
 };
@@ -343,14 +419,21 @@ static void video_i2c_del_list(struct vb2_queue *vq, enum vb2_buffer_state state
 static int start_streaming(struct vb2_queue *vq, unsigned int count)
 {
 	struct video_i2c_data *data = vb2_get_drv_priv(vq);
+	struct device *dev = regmap_get_device(data->regmap);
 	int ret;
 
 	if (data->kthread_vid_cap)
 		return 0;
 
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(dev);
+		goto error_del_list;
+	}
+
 	ret = data->chip->setup(data);
 	if (ret)
-		goto error_del_list;
+		goto error_rpm_put;
 
 	data->sequence = 0;
 	data->kthread_vid_cap = kthread_run(video_i2c_thread_vid_cap, data,
@@ -359,6 +442,9 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 	if (!ret)
 		return 0;
 
+error_rpm_put:
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
 error_del_list:
 	video_i2c_del_list(vq, VB2_BUF_STATE_QUEUED);
 
@@ -374,6 +460,8 @@ static void stop_streaming(struct vb2_queue *vq)
 
 	kthread_stop(data->kthread_vid_cap);
 	data->kthread_vid_cap = NULL;
+	pm_runtime_mark_last_busy(regmap_get_device(data->regmap));
+	pm_runtime_put_autosuspend(regmap_get_device(data->regmap));
 
 	video_i2c_del_list(vq, VB2_BUF_STATE_ERROR);
 }
@@ -569,6 +657,7 @@ static void video_i2c_release(struct video_device *vdev)
 {
 	struct video_i2c_data *data = video_get_drvdata(vdev);
 
+	data->chip->set_power(data, false);
 	v4l2_device_unregister(&data->v4l2_dev);
 	mutex_destroy(&data->lock);
 	mutex_destroy(&data->queue_lock);
@@ -648,6 +737,16 @@ static int video_i2c_probe(struct i2c_client *client,
 	video_set_drvdata(&data->vdev, data);
 	i2c_set_clientdata(client, data);
 
+	ret = data->chip->set_power(data, true);
+	if (ret)
+		goto error_unregister_device;
+
+	pm_runtime_get_noresume(&client->dev);
+	pm_runtime_set_active(&client->dev);
+	pm_runtime_enable(&client->dev);
+	pm_runtime_set_autosuspend_delay(&client->dev, 2000);
+	pm_runtime_use_autosuspend(&client->dev);
+
 	if (data->chip->hwmon_init) {
 		ret = data->chip->hwmon_init(data);
 		if (ret < 0) {
@@ -658,10 +757,19 @@ static int video_i2c_probe(struct i2c_client *client,
 
 	ret = video_register_device(&data->vdev, VFL_TYPE_GRABBER, -1);
 	if (ret < 0)
-		goto error_unregister_device;
+		goto error_pm_disable;
+
+	pm_runtime_mark_last_busy(&client->dev);
+	pm_runtime_put_autosuspend(&client->dev);
 
 	return 0;
 
+error_pm_disable:
+	pm_runtime_disable(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+	pm_runtime_put_noidle(&client->dev);
+	data->chip->set_power(data, false);
+
 error_unregister_device:
 	v4l2_device_unregister(v4l2_dev);
 	mutex_destroy(&data->lock);
@@ -680,11 +788,39 @@ static int video_i2c_remove(struct i2c_client *client)
 {
 	struct video_i2c_data *data = i2c_get_clientdata(client);
 
+	pm_runtime_get_sync(&client->dev);
+	pm_runtime_disable(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+	pm_runtime_put_noidle(&client->dev);
+
 	video_unregister_device(&data->vdev);
 
 	return 0;
 }
 
+#ifdef CONFIG_PM
+
+static int video_i2c_pm_runtime_suspend(struct device *dev)
+{
+	struct video_i2c_data *data = i2c_get_clientdata(to_i2c_client(dev));
+
+	return data->chip->set_power(data, false);
+}
+
+static int video_i2c_pm_runtime_resume(struct device *dev)
+{
+	struct video_i2c_data *data = i2c_get_clientdata(to_i2c_client(dev));
+
+	return data->chip->set_power(data, true);
+}
+
+#endif
+
+static const struct dev_pm_ops video_i2c_pm_ops = {
+	SET_RUNTIME_PM_OPS(video_i2c_pm_runtime_suspend,
+			   video_i2c_pm_runtime_resume, NULL)
+};
+
 static const struct i2c_device_id video_i2c_id_table[] = {
 	{ "amg88xx", AMG88XX },
 	{}
@@ -701,6 +837,7 @@ static struct i2c_driver video_i2c_driver = {
 	.driver = {
 		.name	= VIDEO_I2C_DRIVER,
 		.of_match_table = video_i2c_of_match,
+		.pm	= &video_i2c_pm_ops,
 	},
 	.probe		= video_i2c_probe,
 	.remove		= video_i2c_remove,
-- 
2.7.4

Re: [PATCH v3 1/6] media: video-i2c: avoid accessing released memory area when removing driver

[Matt Ranostay]

On Sat, Oct 13, 2018 at 11:02 AM Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
> The video device release() callback for video-i2c driver frees the whole
> struct video_i2c_data.  If there is no user left for the video device
> when video_unregister_device() is called, the release callback is executed.
>
> However, in video_i2c_remove() some fields (v4l2_dev, lock, and queue_lock)
> in struct video_i2c_data are still accessed after video_unregister_device()
> is called.
>
> This fixes the use after free by moving the code from video_i2c_remove()
> to the release() callback.
>
> Fixes: 5cebaac60974 ("media: video-i2c: add video-i2c driver")
> Cc: Matt Ranostay <matt.ranostay@konsulko.com>

Reviewed-by: Matt Ranostay <matt.ranostay@konsulko.com>

> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Hans Verkuil <hansverk@cisco.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
> * v3
> - Move the code causing use-after-free from video_i2c_remove() to the
>   video device release() callback.
> - Remove Acked-by line as there are enough changes from previous version
>
>  drivers/media/i2c/video-i2c.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index 06d29d8..f27d294 100644
> --- a/drivers/media/i2c/video-i2c.c
> +++ b/drivers/media/i2c/video-i2c.c
> @@ -510,7 +510,12 @@ static const struct v4l2_ioctl_ops video_i2c_ioctl_ops = {
>
>  static void video_i2c_release(struct video_device *vdev)
>  {
> -       kfree(video_get_drvdata(vdev));
> +       struct video_i2c_data *data = video_get_drvdata(vdev);
> +
> +       v4l2_device_unregister(&data->v4l2_dev);
> +       mutex_destroy(&data->lock);
> +       mutex_destroy(&data->queue_lock);
> +       kfree(data);
>  }
>
>  static int video_i2c_probe(struct i2c_client *client,
> @@ -608,10 +613,6 @@ static int video_i2c_remove(struct i2c_client *client)
>         struct video_i2c_data *data = i2c_get_clientdata(client);
>
>         video_unregister_device(&data->vdev);
> -       v4l2_device_unregister(&data->v4l2_dev);
> -
> -       mutex_destroy(&data->lock);
> -       mutex_destroy(&data->queue_lock);
>
>         return 0;
>  }
> --
> 2.7.4
>

Re: [PATCH v3 6/6] media: video-i2c: support runtime PM

[Sakari Ailus]

Hi Mita-san,

On Sun, Oct 14, 2018 at 03:02:39AM +0900, Akinobu Mita wrote:
> AMG88xx has a register for setting operating mode.  This adds support
> runtime PM by changing the operating mode.
> 
> The instruction for changing sleep mode to normal mode is from the
> reference specifications.
> 
> https://docid81hrs3j1.cloudfront.net/medialibrary/2017/11/PANA-S-A0002141979-1.pdf
> 
> Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Hans Verkuil <hansverk@cisco.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Reviewed-by: Matt Ranostay <matt.ranostay@konsulko.com>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
> * v3
> - Move chip->set_power() call to the video device release() callback.
> - Add Acked-by line
> 
>  drivers/media/i2c/video-i2c.c | 141 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 139 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index 3334fc2..22fdc43 100644
> --- a/drivers/media/i2c/video-i2c.c
> +++ b/drivers/media/i2c/video-i2c.c
> @@ -17,6 +17,7 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/of_device.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
> @@ -94,10 +95,23 @@ struct video_i2c_chip {
>  	/* xfer function */
>  	int (*xfer)(struct video_i2c_data *data, char *buf);
>  
> +	/* power control function */
> +	int (*set_power)(struct video_i2c_data *data, bool on);
> +
>  	/* hwmon init function */
>  	int (*hwmon_init)(struct video_i2c_data *data);
>  };
>  
> +/* Power control register */
> +#define AMG88XX_REG_PCTL	0x00
> +#define AMG88XX_PCTL_NORMAL		0x00
> +#define AMG88XX_PCTL_SLEEP		0x10
> +
> +/* Reset register */
> +#define AMG88XX_REG_RST		0x01
> +#define AMG88XX_RST_FLAG		0x30
> +#define AMG88XX_RST_INIT		0x3f
> +
>  /* Frame rate register */
>  #define AMG88XX_REG_FPSC	0x02
>  #define AMG88XX_FPSC_1FPS		BIT(0)
> @@ -127,6 +141,59 @@ static int amg88xx_setup(struct video_i2c_data *data)
>  	return regmap_update_bits(data->regmap, AMG88XX_REG_FPSC, mask, val);
>  }
>  
> +static int amg88xx_set_power_on(struct video_i2c_data *data)
> +{
> +	int ret;
> +
> +	ret = regmap_write(data->regmap, AMG88XX_REG_PCTL, AMG88XX_PCTL_NORMAL);
> +	if (ret)
> +		return ret;
> +
> +	msleep(50);
> +
> +	ret = regmap_write(data->regmap, AMG88XX_REG_RST, AMG88XX_RST_INIT);
> +	if (ret)
> +		return ret;
> +
> +	msleep(2);
> +
> +	ret = regmap_write(data->regmap, AMG88XX_REG_RST, AMG88XX_RST_FLAG);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Wait two frames before reading thermistor and temperature registers
> +	 */
> +	msleep(200);
> +
> +	return 0;
> +}
> +
> +static int amg88xx_set_power_off(struct video_i2c_data *data)
> +{
> +	int ret;
> +
> +	ret = regmap_write(data->regmap, AMG88XX_REG_PCTL, AMG88XX_PCTL_SLEEP);
> +	if (ret)
> +		return ret;
> +	/*
> +	 * Wait for a while to avoid resuming normal mode immediately after
> +	 * entering sleep mode, otherwise the device occasionally goes wrong
> +	 * (thermistor and temperature registers are not updated at all)
> +	 */
> +	msleep(100);
> +
> +	return 0;
> +}
> +
> +static int amg88xx_set_power(struct video_i2c_data *data, bool on)
> +{
> +	if (on)
> +		return amg88xx_set_power_on(data);
> +
> +	return amg88xx_set_power_off(data);
> +}
> +
>  #if IS_ENABLED(CONFIG_HWMON)
>  
>  static const u32 amg88xx_temp_config[] = {
> @@ -158,7 +225,15 @@ static int amg88xx_read(struct device *dev, enum hwmon_sensor_types type,
>  	__le16 buf;
>  	int tmp;
>  
> +	tmp = pm_runtime_get_sync(regmap_get_device(data->regmap));
> +	if (tmp < 0) {
> +		pm_runtime_put_noidle(regmap_get_device(data->regmap));
> +		return tmp;
> +	}
> +
>  	tmp = regmap_bulk_read(data->regmap, AMG88XX_REG_TTHL, &buf, 2);
> +	pm_runtime_mark_last_busy(regmap_get_device(data->regmap));
> +	pm_runtime_put_autosuspend(regmap_get_device(data->regmap));
>  	if (tmp)
>  		return tmp;
>  
> @@ -217,6 +292,7 @@ static const struct video_i2c_chip video_i2c_chip[] = {
>  		.regmap_config	= &amg88xx_regmap_config,
>  		.setup		= &amg88xx_setup,
>  		.xfer		= &amg88xx_xfer,
> +		.set_power	= amg88xx_set_power,
>  		.hwmon_init	= amg88xx_hwmon_init,
>  	},
>  };
> @@ -343,14 +419,21 @@ static void video_i2c_del_list(struct vb2_queue *vq, enum vb2_buffer_state state
>  static int start_streaming(struct vb2_queue *vq, unsigned int count)
>  {
>  	struct video_i2c_data *data = vb2_get_drv_priv(vq);
> +	struct device *dev = regmap_get_device(data->regmap);
>  	int ret;
>  
>  	if (data->kthread_vid_cap)
>  		return 0;
>  
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(dev);
> +		goto error_del_list;
> +	}
> +
>  	ret = data->chip->setup(data);
>  	if (ret)
> -		goto error_del_list;
> +		goto error_rpm_put;
>  
>  	data->sequence = 0;
>  	data->kthread_vid_cap = kthread_run(video_i2c_thread_vid_cap, data,
> @@ -359,6 +442,9 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>  	if (!ret)
>  		return 0;
>  
> +error_rpm_put:
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
>  error_del_list:
>  	video_i2c_del_list(vq, VB2_BUF_STATE_QUEUED);
>  
> @@ -374,6 +460,8 @@ static void stop_streaming(struct vb2_queue *vq)
>  
>  	kthread_stop(data->kthread_vid_cap);
>  	data->kthread_vid_cap = NULL;
> +	pm_runtime_mark_last_busy(regmap_get_device(data->regmap));
> +	pm_runtime_put_autosuspend(regmap_get_device(data->regmap));
>  
>  	video_i2c_del_list(vq, VB2_BUF_STATE_ERROR);
>  }
> @@ -569,6 +657,7 @@ static void video_i2c_release(struct video_device *vdev)
>  {
>  	struct video_i2c_data *data = video_get_drvdata(vdev);
>  
> +	data->chip->set_power(data, false);
>  	v4l2_device_unregister(&data->v4l2_dev);
>  	mutex_destroy(&data->lock);
>  	mutex_destroy(&data->queue_lock);
> @@ -648,6 +737,16 @@ static int video_i2c_probe(struct i2c_client *client,
>  	video_set_drvdata(&data->vdev, data);
>  	i2c_set_clientdata(client, data);
>  
> +	ret = data->chip->set_power(data, true);
> +	if (ret)
> +		goto error_unregister_device;
> +
> +	pm_runtime_get_noresume(&client->dev);
> +	pm_runtime_set_active(&client->dev);
> +	pm_runtime_enable(&client->dev);
> +	pm_runtime_set_autosuspend_delay(&client->dev, 2000);
> +	pm_runtime_use_autosuspend(&client->dev);
> +
>  	if (data->chip->hwmon_init) {
>  		ret = data->chip->hwmon_init(data);
>  		if (ret < 0) {
> @@ -658,10 +757,19 @@ static int video_i2c_probe(struct i2c_client *client,
>  
>  	ret = video_register_device(&data->vdev, VFL_TYPE_GRABBER, -1);
>  	if (ret < 0)
> -		goto error_unregister_device;
> +		goto error_pm_disable;
> +
> +	pm_runtime_mark_last_busy(&client->dev);
> +	pm_runtime_put_autosuspend(&client->dev);
>  
>  	return 0;
>  
> +error_pm_disable:
> +	pm_runtime_disable(&client->dev);
> +	pm_runtime_set_suspended(&client->dev);
> +	pm_runtime_put_noidle(&client->dev);
> +	data->chip->set_power(data, false);
> +
>  error_unregister_device:
>  	v4l2_device_unregister(v4l2_dev);
>  	mutex_destroy(&data->lock);
> @@ -680,11 +788,39 @@ static int video_i2c_remove(struct i2c_client *client)
>  {
>  	struct video_i2c_data *data = i2c_get_clientdata(client);
>  
> +	pm_runtime_get_sync(&client->dev);
> +	pm_runtime_disable(&client->dev);
> +	pm_runtime_set_suspended(&client->dev);
> +	pm_runtime_put_noidle(&client->dev);

The release callback exists so you can release the allocated resources, but
the I²C transactions need to cease after that. So you should call the
set_power() callback here instead --- as you do in probe() function's error
handling.

With that fixed, for the set:

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> +
>  	video_unregister_device(&data->vdev);
>  
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM
> +
> +static int video_i2c_pm_runtime_suspend(struct device *dev)
> +{
> +	struct video_i2c_data *data = i2c_get_clientdata(to_i2c_client(dev));
> +
> +	return data->chip->set_power(data, false);
> +}
> +
> +static int video_i2c_pm_runtime_resume(struct device *dev)
> +{
> +	struct video_i2c_data *data = i2c_get_clientdata(to_i2c_client(dev));
> +
> +	return data->chip->set_power(data, true);
> +}
> +
> +#endif
> +
> +static const struct dev_pm_ops video_i2c_pm_ops = {
> +	SET_RUNTIME_PM_OPS(video_i2c_pm_runtime_suspend,
> +			   video_i2c_pm_runtime_resume, NULL)
> +};
> +
>  static const struct i2c_device_id video_i2c_id_table[] = {
>  	{ "amg88xx", AMG88XX },
>  	{}
> @@ -701,6 +837,7 @@ static struct i2c_driver video_i2c_driver = {
>  	.driver = {
>  		.name	= VIDEO_I2C_DRIVER,
>  		.of_match_table = video_i2c_of_match,
> +		.pm	= &video_i2c_pm_ops,
>  	},
>  	.probe		= video_i2c_probe,
>  	.remove		= video_i2c_remove,

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

Re: [PATCH v3 6/6] media: video-i2c: support runtime PM

[Akinobu Mita]

2018年10月16日(火) 0:31 Sakari Ailus <sakari.ailus@linux.intel.com>:
>
> Hi Mita-san,
>
> On Sun, Oct 14, 2018 at 03:02:39AM +0900, Akinobu Mita wrote:
> > AMG88xx has a register for setting operating mode.  This adds support
> > runtime PM by changing the operating mode.
> >
> > The instruction for changing sleep mode to normal mode is from the
> > reference specifications.
> >
> > https://docid81hrs3j1.cloudfront.net/medialibrary/2017/11/PANA-S-A0002141979-1.pdf
> >
> > Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Cc: Hans Verkuil <hansverk@cisco.com>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > Reviewed-by: Matt Ranostay <matt.ranostay@konsulko.com>
> > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > ---
> > * v3
> > - Move chip->set_power() call to the video device release() callback.
> > - Add Acked-by line
> >
> >  drivers/media/i2c/video-i2c.c | 141 +++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 139 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> > index 3334fc2..22fdc43 100644
> > --- a/drivers/media/i2c/video-i2c.c
> > +++ b/drivers/media/i2c/video-i2c.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> >  #include <linux/of_device.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/regmap.h>
> >  #include <linux/sched.h>
> >  #include <linux/slab.h>
> > @@ -94,10 +95,23 @@ struct video_i2c_chip {
> >       /* xfer function */
> >       int (*xfer)(struct video_i2c_data *data, char *buf);
> >
> > +     /* power control function */
> > +     int (*set_power)(struct video_i2c_data *data, bool on);
> > +
> >       /* hwmon init function */
> >       int (*hwmon_init)(struct video_i2c_data *data);
> >  };
> >
> > +/* Power control register */
> > +#define AMG88XX_REG_PCTL     0x00
> > +#define AMG88XX_PCTL_NORMAL          0x00
> > +#define AMG88XX_PCTL_SLEEP           0x10
> > +
> > +/* Reset register */
> > +#define AMG88XX_REG_RST              0x01
> > +#define AMG88XX_RST_FLAG             0x30
> > +#define AMG88XX_RST_INIT             0x3f
> > +
> >  /* Frame rate register */
> >  #define AMG88XX_REG_FPSC     0x02
> >  #define AMG88XX_FPSC_1FPS            BIT(0)
> > @@ -127,6 +141,59 @@ static int amg88xx_setup(struct video_i2c_data *data)
> >       return regmap_update_bits(data->regmap, AMG88XX_REG_FPSC, mask, val);
> >  }
> >
> > +static int amg88xx_set_power_on(struct video_i2c_data *data)
> > +{
> > +     int ret;
> > +
> > +     ret = regmap_write(data->regmap, AMG88XX_REG_PCTL, AMG88XX_PCTL_NORMAL);
> > +     if (ret)
> > +             return ret;
> > +
> > +     msleep(50);
> > +
> > +     ret = regmap_write(data->regmap, AMG88XX_REG_RST, AMG88XX_RST_INIT);
> > +     if (ret)
> > +             return ret;
> > +
> > +     msleep(2);
> > +
> > +     ret = regmap_write(data->regmap, AMG88XX_REG_RST, AMG88XX_RST_FLAG);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /*
> > +      * Wait two frames before reading thermistor and temperature registers
> > +      */
> > +     msleep(200);
> > +
> > +     return 0;
> > +}
> > +
> > +static int amg88xx_set_power_off(struct video_i2c_data *data)
> > +{
> > +     int ret;
> > +
> > +     ret = regmap_write(data->regmap, AMG88XX_REG_PCTL, AMG88XX_PCTL_SLEEP);
> > +     if (ret)
> > +             return ret;
> > +     /*
> > +      * Wait for a while to avoid resuming normal mode immediately after
> > +      * entering sleep mode, otherwise the device occasionally goes wrong
> > +      * (thermistor and temperature registers are not updated at all)
> > +      */
> > +     msleep(100);
> > +
> > +     return 0;
> > +}
> > +
> > +static int amg88xx_set_power(struct video_i2c_data *data, bool on)
> > +{
> > +     if (on)
> > +             return amg88xx_set_power_on(data);
> > +
> > +     return amg88xx_set_power_off(data);
> > +}
> > +
> >  #if IS_ENABLED(CONFIG_HWMON)
> >
> >  static const u32 amg88xx_temp_config[] = {
> > @@ -158,7 +225,15 @@ static int amg88xx_read(struct device *dev, enum hwmon_sensor_types type,
> >       __le16 buf;
> >       int tmp;
> >
> > +     tmp = pm_runtime_get_sync(regmap_get_device(data->regmap));
> > +     if (tmp < 0) {
> > +             pm_runtime_put_noidle(regmap_get_device(data->regmap));
> > +             return tmp;
> > +     }
> > +
> >       tmp = regmap_bulk_read(data->regmap, AMG88XX_REG_TTHL, &buf, 2);
> > +     pm_runtime_mark_last_busy(regmap_get_device(data->regmap));
> > +     pm_runtime_put_autosuspend(regmap_get_device(data->regmap));
> >       if (tmp)
> >               return tmp;
> >
> > @@ -217,6 +292,7 @@ static const struct video_i2c_chip video_i2c_chip[] = {
> >               .regmap_config  = &amg88xx_regmap_config,
> >               .setup          = &amg88xx_setup,
> >               .xfer           = &amg88xx_xfer,
> > +             .set_power      = amg88xx_set_power,
> >               .hwmon_init     = amg88xx_hwmon_init,
> >       },
> >  };
> > @@ -343,14 +419,21 @@ static void video_i2c_del_list(struct vb2_queue *vq, enum vb2_buffer_state state
> >  static int start_streaming(struct vb2_queue *vq, unsigned int count)
> >  {
> >       struct video_i2c_data *data = vb2_get_drv_priv(vq);
> > +     struct device *dev = regmap_get_device(data->regmap);
> >       int ret;
> >
> >       if (data->kthread_vid_cap)
> >               return 0;
> >
> > +     ret = pm_runtime_get_sync(dev);
> > +     if (ret < 0) {
> > +             pm_runtime_put_noidle(dev);
> > +             goto error_del_list;
> > +     }
> > +
> >       ret = data->chip->setup(data);
> >       if (ret)
> > -             goto error_del_list;
> > +             goto error_rpm_put;
> >
> >       data->sequence = 0;
> >       data->kthread_vid_cap = kthread_run(video_i2c_thread_vid_cap, data,
> > @@ -359,6 +442,9 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
> >       if (!ret)
> >               return 0;
> >
> > +error_rpm_put:
> > +     pm_runtime_mark_last_busy(dev);
> > +     pm_runtime_put_autosuspend(dev);
> >  error_del_list:
> >       video_i2c_del_list(vq, VB2_BUF_STATE_QUEUED);
> >
> > @@ -374,6 +460,8 @@ static void stop_streaming(struct vb2_queue *vq)
> >
> >       kthread_stop(data->kthread_vid_cap);
> >       data->kthread_vid_cap = NULL;
> > +     pm_runtime_mark_last_busy(regmap_get_device(data->regmap));
> > +     pm_runtime_put_autosuspend(regmap_get_device(data->regmap));
> >
> >       video_i2c_del_list(vq, VB2_BUF_STATE_ERROR);
> >  }
> > @@ -569,6 +657,7 @@ static void video_i2c_release(struct video_device *vdev)
> >  {
> >       struct video_i2c_data *data = video_get_drvdata(vdev);
> >
> > +     data->chip->set_power(data, false);
> >       v4l2_device_unregister(&data->v4l2_dev);
> >       mutex_destroy(&data->lock);
> >       mutex_destroy(&data->queue_lock);
> > @@ -648,6 +737,16 @@ static int video_i2c_probe(struct i2c_client *client,
> >       video_set_drvdata(&data->vdev, data);
> >       i2c_set_clientdata(client, data);
> >
> > +     ret = data->chip->set_power(data, true);
> > +     if (ret)
> > +             goto error_unregister_device;
> > +
> > +     pm_runtime_get_noresume(&client->dev);
> > +     pm_runtime_set_active(&client->dev);
> > +     pm_runtime_enable(&client->dev);
> > +     pm_runtime_set_autosuspend_delay(&client->dev, 2000);
> > +     pm_runtime_use_autosuspend(&client->dev);
> > +
> >       if (data->chip->hwmon_init) {
> >               ret = data->chip->hwmon_init(data);
> >               if (ret < 0) {
> > @@ -658,10 +757,19 @@ static int video_i2c_probe(struct i2c_client *client,
> >
> >       ret = video_register_device(&data->vdev, VFL_TYPE_GRABBER, -1);
> >       if (ret < 0)
> > -             goto error_unregister_device;
> > +             goto error_pm_disable;
> > +
> > +     pm_runtime_mark_last_busy(&client->dev);
> > +     pm_runtime_put_autosuspend(&client->dev);
> >
> >       return 0;
> >
> > +error_pm_disable:
> > +     pm_runtime_disable(&client->dev);
> > +     pm_runtime_set_suspended(&client->dev);
> > +     pm_runtime_put_noidle(&client->dev);
> > +     data->chip->set_power(data, false);
> > +
> >  error_unregister_device:
> >       v4l2_device_unregister(v4l2_dev);
> >       mutex_destroy(&data->lock);
> > @@ -680,11 +788,39 @@ static int video_i2c_remove(struct i2c_client *client)
> >  {
> >       struct video_i2c_data *data = i2c_get_clientdata(client);
> >
> > +     pm_runtime_get_sync(&client->dev);
> > +     pm_runtime_disable(&client->dev);
> > +     pm_runtime_set_suspended(&client->dev);
> > +     pm_runtime_put_noidle(&client->dev);
>
> The release callback exists so you can release the allocated resources, but
> the I涎 transactions need to cease after that. So you should call the
> set_power() callback here instead --- as you do in probe() function's error
> handling.

Hi Sakari,

The set_power() callback is called in video_i2c_release() release
callback in this patch, so it should be the last I2C transaction.

case a.1)  When the driver is unbound, no users grab a file handle.

video_i2c_remove
 |
 -> pm_runtime_*
 -> video_unregister_device
     :
     -> video_i2c_release
         |
         -> data->chip->set_power
         -> v4l2_device_unregister
         -> mutex_destroy
         -> regmap_exit(data->regmap);
         -> kfree

case a.2)  When the driver is unbound, some users grab a file handle.

video_i2c_remove
 |
 -> pm_runtime_*
 -> video_unregister_device

<All users ungrab a file handle>

video_i2c_release
 |
 -> data->chip->set_power
 -> v4l2_device_unregister
 -> mutex_destroy
 -> regmap_exit(data->regmap);
 -> kfree

If the set_power() callback is moved to video_i2c_remove() as you
suggested, it doesn't ensure set_power() callback is the last I2C
transaction in case b.2), does it?

case b.1)  When the driver is unbound, no users grab a file handle.

video_i2c_remove
 |
 -> pm_runtime_*
 -> data->chip->set_power
 -> video_unregister_device
     :
     -> video_i2c_release
         |
         -> v4l2_device_unregister
         -> mutex_destroy
         -> regmap_exit(data->regmap);
         -> kfree

case b.2)  When the driver is unbound, some users grab a file handle.

video_i2c_remove
 |
 -> pm_runtime_*
 -> data->chip->set_power
 -> video_unregister_device

<All users ungrab a file handle>

video_i2c_release
 |
 -> v4l2_device_unregister
 -> mutex_destroy
 -> regmap_exit(data->regmap);
 -> kfree

Re: [PATCH v3 6/6] media: video-i2c: support runtime PM

[Sakari Ailus]

On Wed, Oct 17, 2018 at 12:07:50AM +0900, Akinobu Mita wrote:
> 2018年10月16日(火) 0:31 Sakari Ailus <sakari.ailus@linux.intel.com>:
> >
> > Hi Mita-san,
> >
> > On Sun, Oct 14, 2018 at 03:02:39AM +0900, Akinobu Mita wrote:
> > > AMG88xx has a register for setting operating mode.  This adds support
> > > runtime PM by changing the operating mode.
> > >
> > > The instruction for changing sleep mode to normal mode is from the
> > > reference specifications.
> > >
> > > https://docid81hrs3j1.cloudfront.net/medialibrary/2017/11/PANA-S-A0002141979-1.pdf
> > >
> > > Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> > > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Cc: Hans Verkuil <hansverk@cisco.com>
> > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > > Reviewed-by: Matt Ranostay <matt.ranostay@konsulko.com>
> > > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > > ---
> > > * v3
> > > - Move chip->set_power() call to the video device release() callback.
> > > - Add Acked-by line
> > >
> > >  drivers/media/i2c/video-i2c.c | 141 +++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 139 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> > > index 3334fc2..22fdc43 100644
> > > --- a/drivers/media/i2c/video-i2c.c
> > > +++ b/drivers/media/i2c/video-i2c.c
> > > @@ -17,6 +17,7 @@
> > >  #include <linux/module.h>
> > >  #include <linux/mutex.h>
> > >  #include <linux/of_device.h>
> > > +#include <linux/pm_runtime.h>
> > >  #include <linux/regmap.h>
> > >  #include <linux/sched.h>
> > >  #include <linux/slab.h>
> > > @@ -94,10 +95,23 @@ struct video_i2c_chip {
> > >       /* xfer function */
> > >       int (*xfer)(struct video_i2c_data *data, char *buf);
> > >
> > > +     /* power control function */
> > > +     int (*set_power)(struct video_i2c_data *data, bool on);
> > > +
> > >       /* hwmon init function */
> > >       int (*hwmon_init)(struct video_i2c_data *data);
> > >  };
> > >
> > > +/* Power control register */
> > > +#define AMG88XX_REG_PCTL     0x00
> > > +#define AMG88XX_PCTL_NORMAL          0x00
> > > +#define AMG88XX_PCTL_SLEEP           0x10
> > > +
> > > +/* Reset register */
> > > +#define AMG88XX_REG_RST              0x01
> > > +#define AMG88XX_RST_FLAG             0x30
> > > +#define AMG88XX_RST_INIT             0x3f
> > > +
> > >  /* Frame rate register */
> > >  #define AMG88XX_REG_FPSC     0x02
> > >  #define AMG88XX_FPSC_1FPS            BIT(0)
> > > @@ -127,6 +141,59 @@ static int amg88xx_setup(struct video_i2c_data *data)
> > >       return regmap_update_bits(data->regmap, AMG88XX_REG_FPSC, mask, val);
> > >  }
> > >
> > > +static int amg88xx_set_power_on(struct video_i2c_data *data)
> > > +{
> > > +     int ret;
> > > +
> > > +     ret = regmap_write(data->regmap, AMG88XX_REG_PCTL, AMG88XX_PCTL_NORMAL);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     msleep(50);
> > > +
> > > +     ret = regmap_write(data->regmap, AMG88XX_REG_RST, AMG88XX_RST_INIT);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     msleep(2);
> > > +
> > > +     ret = regmap_write(data->regmap, AMG88XX_REG_RST, AMG88XX_RST_FLAG);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     /*
> > > +      * Wait two frames before reading thermistor and temperature registers
> > > +      */
> > > +     msleep(200);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int amg88xx_set_power_off(struct video_i2c_data *data)
> > > +{
> > > +     int ret;
> > > +
> > > +     ret = regmap_write(data->regmap, AMG88XX_REG_PCTL, AMG88XX_PCTL_SLEEP);
> > > +     if (ret)
> > > +             return ret;
> > > +     /*
> > > +      * Wait for a while to avoid resuming normal mode immediately after
> > > +      * entering sleep mode, otherwise the device occasionally goes wrong
> > > +      * (thermistor and temperature registers are not updated at all)
> > > +      */
> > > +     msleep(100);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int amg88xx_set_power(struct video_i2c_data *data, bool on)
> > > +{
> > > +     if (on)
> > > +             return amg88xx_set_power_on(data);
> > > +
> > > +     return amg88xx_set_power_off(data);
> > > +}
> > > +
> > >  #if IS_ENABLED(CONFIG_HWMON)
> > >
> > >  static const u32 amg88xx_temp_config[] = {
> > > @@ -158,7 +225,15 @@ static int amg88xx_read(struct device *dev, enum hwmon_sensor_types type,
> > >       __le16 buf;
> > >       int tmp;
> > >
> > > +     tmp = pm_runtime_get_sync(regmap_get_device(data->regmap));
> > > +     if (tmp < 0) {
> > > +             pm_runtime_put_noidle(regmap_get_device(data->regmap));
> > > +             return tmp;
> > > +     }
> > > +
> > >       tmp = regmap_bulk_read(data->regmap, AMG88XX_REG_TTHL, &buf, 2);
> > > +     pm_runtime_mark_last_busy(regmap_get_device(data->regmap));
> > > +     pm_runtime_put_autosuspend(regmap_get_device(data->regmap));
> > >       if (tmp)
> > >               return tmp;
> > >
> > > @@ -217,6 +292,7 @@ static const struct video_i2c_chip video_i2c_chip[] = {
> > >               .regmap_config  = &amg88xx_regmap_config,
> > >               .setup          = &amg88xx_setup,
> > >               .xfer           = &amg88xx_xfer,
> > > +             .set_power      = amg88xx_set_power,
> > >               .hwmon_init     = amg88xx_hwmon_init,
> > >       },
> > >  };
> > > @@ -343,14 +419,21 @@ static void video_i2c_del_list(struct vb2_queue *vq, enum vb2_buffer_state state
> > >  static int start_streaming(struct vb2_queue *vq, unsigned int count)
> > >  {
> > >       struct video_i2c_data *data = vb2_get_drv_priv(vq);
> > > +     struct device *dev = regmap_get_device(data->regmap);
> > >       int ret;
> > >
> > >       if (data->kthread_vid_cap)
> > >               return 0;
> > >
> > > +     ret = pm_runtime_get_sync(dev);
> > > +     if (ret < 0) {
> > > +             pm_runtime_put_noidle(dev);
> > > +             goto error_del_list;
> > > +     }
> > > +
> > >       ret = data->chip->setup(data);
> > >       if (ret)
> > > -             goto error_del_list;
> > > +             goto error_rpm_put;
> > >
> > >       data->sequence = 0;
> > >       data->kthread_vid_cap = kthread_run(video_i2c_thread_vid_cap, data,
> > > @@ -359,6 +442,9 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
> > >       if (!ret)
> > >               return 0;
> > >
> > > +error_rpm_put:
> > > +     pm_runtime_mark_last_busy(dev);
> > > +     pm_runtime_put_autosuspend(dev);
> > >  error_del_list:
> > >       video_i2c_del_list(vq, VB2_BUF_STATE_QUEUED);
> > >
> > > @@ -374,6 +460,8 @@ static void stop_streaming(struct vb2_queue *vq)
> > >
> > >       kthread_stop(data->kthread_vid_cap);
> > >       data->kthread_vid_cap = NULL;
> > > +     pm_runtime_mark_last_busy(regmap_get_device(data->regmap));
> > > +     pm_runtime_put_autosuspend(regmap_get_device(data->regmap));
> > >
> > >       video_i2c_del_list(vq, VB2_BUF_STATE_ERROR);
> > >  }
> > > @@ -569,6 +657,7 @@ static void video_i2c_release(struct video_device *vdev)
> > >  {
> > >       struct video_i2c_data *data = video_get_drvdata(vdev);
> > >
> > > +     data->chip->set_power(data, false);
> > >       v4l2_device_unregister(&data->v4l2_dev);
> > >       mutex_destroy(&data->lock);
> > >       mutex_destroy(&data->queue_lock);
> > > @@ -648,6 +737,16 @@ static int video_i2c_probe(struct i2c_client *client,
> > >       video_set_drvdata(&data->vdev, data);
> > >       i2c_set_clientdata(client, data);
> > >
> > > +     ret = data->chip->set_power(data, true);
> > > +     if (ret)
> > > +             goto error_unregister_device;
> > > +
> > > +     pm_runtime_get_noresume(&client->dev);
> > > +     pm_runtime_set_active(&client->dev);
> > > +     pm_runtime_enable(&client->dev);
> > > +     pm_runtime_set_autosuspend_delay(&client->dev, 2000);
> > > +     pm_runtime_use_autosuspend(&client->dev);
> > > +
> > >       if (data->chip->hwmon_init) {
> > >               ret = data->chip->hwmon_init(data);
> > >               if (ret < 0) {
> > > @@ -658,10 +757,19 @@ static int video_i2c_probe(struct i2c_client *client,
> > >
> > >       ret = video_register_device(&data->vdev, VFL_TYPE_GRABBER, -1);
> > >       if (ret < 0)
> > > -             goto error_unregister_device;
> > > +             goto error_pm_disable;
> > > +
> > > +     pm_runtime_mark_last_busy(&client->dev);
> > > +     pm_runtime_put_autosuspend(&client->dev);
> > >
> > >       return 0;
> > >
> > > +error_pm_disable:
> > > +     pm_runtime_disable(&client->dev);
> > > +     pm_runtime_set_suspended(&client->dev);
> > > +     pm_runtime_put_noidle(&client->dev);
> > > +     data->chip->set_power(data, false);
> > > +
> > >  error_unregister_device:
> > >       v4l2_device_unregister(v4l2_dev);
> > >       mutex_destroy(&data->lock);
> > > @@ -680,11 +788,39 @@ static int video_i2c_remove(struct i2c_client *client)
> > >  {
> > >       struct video_i2c_data *data = i2c_get_clientdata(client);
> > >
> > > +     pm_runtime_get_sync(&client->dev);
> > > +     pm_runtime_disable(&client->dev);
> > > +     pm_runtime_set_suspended(&client->dev);
> > > +     pm_runtime_put_noidle(&client->dev);
> >
> > The release callback exists so you can release the allocated resources, but
> > the I涎 transactions need to cease after that. So you should call the
> > set_power() callback here instead --- as you do in probe() function's error
> > handling.
> 
> Hi Sakari,
> 
> The set_power() callback is called in video_i2c_release() release
> callback in this patch, so it should be the last I2C transaction.

You can no longer access the device once the remove callback has finished.
If you need to do access it, it's there, not later.

> 
> case a.1)  When the driver is unbound, no users grab a file handle.
> 
> video_i2c_remove
>  |
>  -> pm_runtime_*
>  -> video_unregister_device
>      :
>      -> video_i2c_release
>          |
>          -> data->chip->set_power
>          -> v4l2_device_unregister
>          -> mutex_destroy
>          -> regmap_exit(data->regmap);
>          -> kfree
> 
> case a.2)  When the driver is unbound, some users grab a file handle.
> 
> video_i2c_remove
>  |
>  -> pm_runtime_*
>  -> video_unregister_device
> 
> <All users ungrab a file handle>
> 
> video_i2c_release
>  |
>  -> data->chip->set_power
>  -> v4l2_device_unregister
>  -> mutex_destroy
>  -> regmap_exit(data->regmap);
>  -> kfree
> 
> If the set_power() callback is moved to video_i2c_remove() as you
> suggested, it doesn't ensure set_power() callback is the last I2C
> transaction in case b.2), does it?

That's something the driver would need to ensure in principle. The I²C core
doesn't seem to help there either. In I²C case I presume you'd get an error
if the device was already powered down by that point but on other busses
you might get a system crash.

> 
> case b.1)  When the driver is unbound, no users grab a file handle.
> 
> video_i2c_remove
>  |
>  -> pm_runtime_*
>  -> data->chip->set_power
>  -> video_unregister_device
>      :
>      -> video_i2c_release
>          |
>          -> v4l2_device_unregister
>          -> mutex_destroy
>          -> regmap_exit(data->regmap);
>          -> kfree
> 
> case b.2)  When the driver is unbound, some users grab a file handle.
> 
> video_i2c_remove
>  |
>  -> pm_runtime_*
>  -> data->chip->set_power
>  -> video_unregister_device
> 
> <All users ungrab a file handle>
> 
> video_i2c_release
>  |
>  -> v4l2_device_unregister
>  -> mutex_destroy
>  -> regmap_exit(data->regmap);
>  -> kfree

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v3 6/6] media: video-i2c: support runtime PM

[Akinobu Mita]

2018年10月17日(水) 16:19 Sakari Ailus <sakari.ailus@linux.intel.com>:
>
> On Wed, Oct 17, 2018 at 12:07:50AM +0900, Akinobu Mita wrote:
> > 2018年10月16日(火) 0:31 Sakari Ailus <sakari.ailus@linux.intel.com>:
> > >
> > > Hi Mita-san,
> > >
> > > On Sun, Oct 14, 2018 at 03:02:39AM +0900, Akinobu Mita wrote:
> > > > AMG88xx has a register for setting operating mode.  This adds support
> > > > runtime PM by changing the operating mode.
> > > >
> > > > The instruction for changing sleep mode to normal mode is from the
> > > > reference specifications.
> > > >
> > > > https://docid81hrs3j1.cloudfront.net/medialibrary/2017/11/PANA-S-A0002141979-1.pdf
> > > >
> > > > Cc: Matt Ranostay <matt.ranostay@konsulko.com>
> > > > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > Cc: Hans Verkuil <hansverk@cisco.com>
> > > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > > > Reviewed-by: Matt Ranostay <matt.ranostay@konsulko.com>
> > > > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > > > ---
> > > > * v3
> > > > - Move chip->set_power() call to the video device release() callback.
> > > > - Add Acked-by line
> > > >
> > > >  drivers/media/i2c/video-i2c.c | 141 +++++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 139 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> > > > index 3334fc2..22fdc43 100644
> > > > --- a/drivers/media/i2c/video-i2c.c
> > > > +++ b/drivers/media/i2c/video-i2c.c
> > > > @@ -17,6 +17,7 @@
> > > >  #include <linux/module.h>
> > > >  #include <linux/mutex.h>
> > > >  #include <linux/of_device.h>
> > > > +#include <linux/pm_runtime.h>
> > > >  #include <linux/regmap.h>
> > > >  #include <linux/sched.h>
> > > >  #include <linux/slab.h>
> > > > @@ -94,10 +95,23 @@ struct video_i2c_chip {
> > > >       /* xfer function */
> > > >       int (*xfer)(struct video_i2c_data *data, char *buf);
> > > >
> > > > +     /* power control function */
> > > > +     int (*set_power)(struct video_i2c_data *data, bool on);
> > > > +
> > > >       /* hwmon init function */
> > > >       int (*hwmon_init)(struct video_i2c_data *data);
> > > >  };
> > > >
> > > > +/* Power control register */
> > > > +#define AMG88XX_REG_PCTL     0x00
> > > > +#define AMG88XX_PCTL_NORMAL          0x00
> > > > +#define AMG88XX_PCTL_SLEEP           0x10
> > > > +
> > > > +/* Reset register */
> > > > +#define AMG88XX_REG_RST              0x01
> > > > +#define AMG88XX_RST_FLAG             0x30
> > > > +#define AMG88XX_RST_INIT             0x3f
> > > > +
> > > >  /* Frame rate register */
> > > >  #define AMG88XX_REG_FPSC     0x02
> > > >  #define AMG88XX_FPSC_1FPS            BIT(0)
> > > > @@ -127,6 +141,59 @@ static int amg88xx_setup(struct video_i2c_data *data)
> > > >       return regmap_update_bits(data->regmap, AMG88XX_REG_FPSC, mask, val);
> > > >  }
> > > >
> > > > +static int amg88xx_set_power_on(struct video_i2c_data *data)
> > > > +{
> > > > +     int ret;
> > > > +
> > > > +     ret = regmap_write(data->regmap, AMG88XX_REG_PCTL, AMG88XX_PCTL_NORMAL);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     msleep(50);
> > > > +
> > > > +     ret = regmap_write(data->regmap, AMG88XX_REG_RST, AMG88XX_RST_INIT);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     msleep(2);
> > > > +
> > > > +     ret = regmap_write(data->regmap, AMG88XX_REG_RST, AMG88XX_RST_FLAG);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     /*
> > > > +      * Wait two frames before reading thermistor and temperature registers
> > > > +      */
> > > > +     msleep(200);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int amg88xx_set_power_off(struct video_i2c_data *data)
> > > > +{
> > > > +     int ret;
> > > > +
> > > > +     ret = regmap_write(data->regmap, AMG88XX_REG_PCTL, AMG88XX_PCTL_SLEEP);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +     /*
> > > > +      * Wait for a while to avoid resuming normal mode immediately after
> > > > +      * entering sleep mode, otherwise the device occasionally goes wrong
> > > > +      * (thermistor and temperature registers are not updated at all)
> > > > +      */
> > > > +     msleep(100);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int amg88xx_set_power(struct video_i2c_data *data, bool on)
> > > > +{
> > > > +     if (on)
> > > > +             return amg88xx_set_power_on(data);
> > > > +
> > > > +     return amg88xx_set_power_off(data);
> > > > +}
> > > > +
> > > >  #if IS_ENABLED(CONFIG_HWMON)
> > > >
> > > >  static const u32 amg88xx_temp_config[] = {
> > > > @@ -158,7 +225,15 @@ static int amg88xx_read(struct device *dev, enum hwmon_sensor_types type,
> > > >       __le16 buf;
> > > >       int tmp;
> > > >
> > > > +     tmp = pm_runtime_get_sync(regmap_get_device(data->regmap));
> > > > +     if (tmp < 0) {
> > > > +             pm_runtime_put_noidle(regmap_get_device(data->regmap));
> > > > +             return tmp;
> > > > +     }
> > > > +
> > > >       tmp = regmap_bulk_read(data->regmap, AMG88XX_REG_TTHL, &buf, 2);
> > > > +     pm_runtime_mark_last_busy(regmap_get_device(data->regmap));
> > > > +     pm_runtime_put_autosuspend(regmap_get_device(data->regmap));
> > > >       if (tmp)
> > > >               return tmp;
> > > >
> > > > @@ -217,6 +292,7 @@ static const struct video_i2c_chip video_i2c_chip[] = {
> > > >               .regmap_config  = &amg88xx_regmap_config,
> > > >               .setup          = &amg88xx_setup,
> > > >               .xfer           = &amg88xx_xfer,
> > > > +             .set_power      = amg88xx_set_power,
> > > >               .hwmon_init     = amg88xx_hwmon_init,
> > > >       },
> > > >  };
> > > > @@ -343,14 +419,21 @@ static void video_i2c_del_list(struct vb2_queue *vq, enum vb2_buffer_state state
> > > >  static int start_streaming(struct vb2_queue *vq, unsigned int count)
> > > >  {
> > > >       struct video_i2c_data *data = vb2_get_drv_priv(vq);
> > > > +     struct device *dev = regmap_get_device(data->regmap);
> > > >       int ret;
> > > >
> > > >       if (data->kthread_vid_cap)
> > > >               return 0;
> > > >
> > > > +     ret = pm_runtime_get_sync(dev);
> > > > +     if (ret < 0) {
> > > > +             pm_runtime_put_noidle(dev);
> > > > +             goto error_del_list;
> > > > +     }
> > > > +
> > > >       ret = data->chip->setup(data);
> > > >       if (ret)
> > > > -             goto error_del_list;
> > > > +             goto error_rpm_put;
> > > >
> > > >       data->sequence = 0;
> > > >       data->kthread_vid_cap = kthread_run(video_i2c_thread_vid_cap, data,
> > > > @@ -359,6 +442,9 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
> > > >       if (!ret)
> > > >               return 0;
> > > >
> > > > +error_rpm_put:
> > > > +     pm_runtime_mark_last_busy(dev);
> > > > +     pm_runtime_put_autosuspend(dev);
> > > >  error_del_list:
> > > >       video_i2c_del_list(vq, VB2_BUF_STATE_QUEUED);
> > > >
> > > > @@ -374,6 +460,8 @@ static void stop_streaming(struct vb2_queue *vq)
> > > >
> > > >       kthread_stop(data->kthread_vid_cap);
> > > >       data->kthread_vid_cap = NULL;
> > > > +     pm_runtime_mark_last_busy(regmap_get_device(data->regmap));
> > > > +     pm_runtime_put_autosuspend(regmap_get_device(data->regmap));
> > > >
> > > >       video_i2c_del_list(vq, VB2_BUF_STATE_ERROR);
> > > >  }
> > > > @@ -569,6 +657,7 @@ static void video_i2c_release(struct video_device *vdev)
> > > >  {
> > > >       struct video_i2c_data *data = video_get_drvdata(vdev);
> > > >
> > > > +     data->chip->set_power(data, false);
> > > >       v4l2_device_unregister(&data->v4l2_dev);
> > > >       mutex_destroy(&data->lock);
> > > >       mutex_destroy(&data->queue_lock);
> > > > @@ -648,6 +737,16 @@ static int video_i2c_probe(struct i2c_client *client,
> > > >       video_set_drvdata(&data->vdev, data);
> > > >       i2c_set_clientdata(client, data);
> > > >
> > > > +     ret = data->chip->set_power(data, true);
> > > > +     if (ret)
> > > > +             goto error_unregister_device;
> > > > +
> > > > +     pm_runtime_get_noresume(&client->dev);
> > > > +     pm_runtime_set_active(&client->dev);
> > > > +     pm_runtime_enable(&client->dev);
> > > > +     pm_runtime_set_autosuspend_delay(&client->dev, 2000);
> > > > +     pm_runtime_use_autosuspend(&client->dev);
> > > > +
> > > >       if (data->chip->hwmon_init) {
> > > >               ret = data->chip->hwmon_init(data);
> > > >               if (ret < 0) {
> > > > @@ -658,10 +757,19 @@ static int video_i2c_probe(struct i2c_client *client,
> > > >
> > > >       ret = video_register_device(&data->vdev, VFL_TYPE_GRABBER, -1);
> > > >       if (ret < 0)
> > > > -             goto error_unregister_device;
> > > > +             goto error_pm_disable;
> > > > +
> > > > +     pm_runtime_mark_last_busy(&client->dev);
> > > > +     pm_runtime_put_autosuspend(&client->dev);
> > > >
> > > >       return 0;
> > > >
> > > > +error_pm_disable:
> > > > +     pm_runtime_disable(&client->dev);
> > > > +     pm_runtime_set_suspended(&client->dev);
> > > > +     pm_runtime_put_noidle(&client->dev);
> > > > +     data->chip->set_power(data, false);
> > > > +
> > > >  error_unregister_device:
> > > >       v4l2_device_unregister(v4l2_dev);
> > > >       mutex_destroy(&data->lock);
> > > > @@ -680,11 +788,39 @@ static int video_i2c_remove(struct i2c_client *client)
> > > >  {
> > > >       struct video_i2c_data *data = i2c_get_clientdata(client);
> > > >
> > > > +     pm_runtime_get_sync(&client->dev);
> > > > +     pm_runtime_disable(&client->dev);
> > > > +     pm_runtime_set_suspended(&client->dev);
> > > > +     pm_runtime_put_noidle(&client->dev);
> > >
> > > The release callback exists so you can release the allocated resources, but
> > > the I涎 transactions need to cease after that. So you should call the
> > > set_power() callback here instead --- as you do in probe() function's error
> > > handling.
> >
> > Hi Sakari,
> >
> > The set_power() callback is called in video_i2c_release() release
> > callback in this patch, so it should be the last I2C transaction.
>
> You can no longer access the device once the remove callback has finished.
> If you need to do access it, it's there, not later.

OK, I see.

> >
> > case a.1)  When the driver is unbound, no users grab a file handle.
> >
> > video_i2c_remove
> >  |
> >  -> pm_runtime_*
> >  -> video_unregister_device
> >      :
> >      -> video_i2c_release
> >          |
> >          -> data->chip->set_power
> >          -> v4l2_device_unregister
> >          -> mutex_destroy
> >          -> regmap_exit(data->regmap);
> >          -> kfree
> >
> > case a.2)  When the driver is unbound, some users grab a file handle.
> >
> > video_i2c_remove
> >  |
> >  -> pm_runtime_*
> >  -> video_unregister_device
> >
> > <All users ungrab a file handle>
> >
> > video_i2c_release
> >  |
> >  -> data->chip->set_power
> >  -> v4l2_device_unregister
> >  -> mutex_destroy
> >  -> regmap_exit(data->regmap);
> >  -> kfree
> >
> > If the set_power() callback is moved to video_i2c_remove() as you
> > suggested, it doesn't ensure set_power() callback is the last I2C
> > transaction in case b.2), does it?
>
> That's something the driver would need to ensure in principle. The I²C core
> doesn't seem to help there either. In I²C case I presume you'd get an error
> if the device was already powered down by that point but on other busses
> you might get a system crash.

Thanks for the explanation. I'll move the set_power() call  to
video_i2c_remove() in v4.

> >
> > case b.1)  When the driver is unbound, no users grab a file handle.
> >
> > video_i2c_remove
> >  |
> >  -> pm_runtime_*
> >  -> data->chip->set_power
> >  -> video_unregister_device
> >      :
> >      -> video_i2c_release
> >          |
> >          -> v4l2_device_unregister
> >          -> mutex_destroy
> >          -> regmap_exit(data->regmap);
> >          -> kfree
> >
> > case b.2)  When the driver is unbound, some users grab a file handle.
> >
> > video_i2c_remove
> >  |
> >  -> pm_runtime_*
> >  -> data->chip->set_power
> >  -> video_unregister_device
> >
> > <All users ungrab a file handle>
> >
> > video_i2c_release
> >  |
> >  -> v4l2_device_unregister
> >  -> mutex_destroy
> >  -> regmap_exit(data->regmap);
> >  -> kfree
>
> --
> Kind regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com