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

[PATCH 0/5] 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 function 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.

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

 drivers/media/i2c/video-i2c.c         | 276 ++++++++++++++++++++++++++++------
 drivers/media/v4l2-core/v4l2-common.c |  26 ++++
 include/media/v4l2-common.h           |  12 ++
 3 files changed, 267 insertions(+), 47 deletions(-)

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 1/5] media: video-i2c: avoid accessing released memory area when removing driver

[Akinobu Mita]

The struct video_i2c_data is released when video_unregister_device() is
called, but it will still be accessed after calling
video_unregister_device().

Use devm_kzalloc() and let the memory be automatically released on driver
detach.

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>
---
 drivers/media/i2c/video-i2c.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index 06d29d8..b7a2af9 100644
--- a/drivers/media/i2c/video-i2c.c
+++ b/drivers/media/i2c/video-i2c.c
@@ -508,20 +508,15 @@ static const struct v4l2_ioctl_ops video_i2c_ioctl_ops = {
 	.vidioc_streamoff		= vb2_ioctl_streamoff,
 };
 
-static void video_i2c_release(struct video_device *vdev)
-{
-	kfree(video_get_drvdata(vdev));
-}
-
 static int video_i2c_probe(struct i2c_client *client,
 			     const struct i2c_device_id *id)
 {
 	struct video_i2c_data *data;
 	struct v4l2_device *v4l2_dev;
 	struct vb2_queue *queue;
-	int ret = -ENODEV;
+	int ret;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
@@ -530,7 +525,7 @@ static int video_i2c_probe(struct i2c_client *client,
 	else if (id)
 		data->chip = &video_i2c_chip[id->driver_data];
 	else
-		goto error_free_device;
+		return -ENODEV;
 
 	data->client = client;
 	v4l2_dev = &data->v4l2_dev;
@@ -538,7 +533,7 @@ static int video_i2c_probe(struct i2c_client *client,
 
 	ret = v4l2_device_register(&client->dev, v4l2_dev);
 	if (ret < 0)
-		goto error_free_device;
+		return ret;
 
 	mutex_init(&data->lock);
 	mutex_init(&data->queue_lock);
@@ -568,7 +563,7 @@ static int video_i2c_probe(struct i2c_client *client,
 	data->vdev.fops = &video_i2c_fops;
 	data->vdev.lock = &data->lock;
 	data->vdev.ioctl_ops = &video_i2c_ioctl_ops;
-	data->vdev.release = video_i2c_release;
+	data->vdev.release = video_device_release_empty;
 	data->vdev.device_caps = V4L2_CAP_VIDEO_CAPTURE |
 				 V4L2_CAP_READWRITE | V4L2_CAP_STREAMING;
 
@@ -597,9 +592,6 @@ static int video_i2c_probe(struct i2c_client *client,
 	mutex_destroy(&data->lock);
 	mutex_destroy(&data->queue_lock);
 
-error_free_device:
-	kfree(data);
-
 	return ret;
 }
 
-- 
2.7.4

[PATCH 2/5] 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>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/media/i2c/video-i2c.c | 54 ++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 26 deletions(-)

diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index b7a2af9..90d389b 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);
 
@@ -85,24 +94,8 @@ struct video_i2c_chip {
 
 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;
-
-	msg[1].addr = client->addr;
-	msg[1].flags = I2C_M_RD;
-	msg[1].len = data->chip->buffer_size;
-	msg[1].buf = (char *)buf;
-
-	ret = i2c_transfer(client->adapter, msg, 2);
-
-	return (ret == 2) ? 0 : -EIO;
+	return regmap_bulk_read(data->regmap, 0x80, buf,
+				data->chip->buffer_size);
 }
 
 #if IS_ENABLED(CONFIG_HWMON)
@@ -133,12 +126,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, 0x0e, &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 +160,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 +179,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 +348,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));
@@ -527,7 +526,10 @@ static int video_i2c_probe(struct i2c_client *client,
 	else
 		return -ENODEV;
 
-	data->client = client;
+	data->regmap = devm_regmap_init_i2c(client, data->chip->regmap_config);
+	if (IS_ERR(data->regmap))
+		return PTR_ERR(data->regmap);
+
 	v4l2_dev = &data->v4l2_dev;
 	strlcpy(v4l2_dev->name, VIDEO_I2C_DRIVER, sizeof(v4l2_dev->name));
 
-- 
2.7.4

[PATCH 3/5] media: v4l2-common: add v4l2_find_closest_fract()

[Akinobu Mita]

Add a function to locate the closest element in a sorted v4l2_fract array.

The implementation is based on find_closest() macro in linux/util_macros.h
and the way to compare two v4l2_fract in vivid_vid_cap_s_parm in
drivers/media/platform/vivid/vivid-vid-cap.c.

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>
---
 drivers/media/v4l2-core/v4l2-common.c | 26 ++++++++++++++++++++++++++
 include/media/v4l2-common.h           | 12 ++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index b518b92..91bd460 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -387,6 +387,32 @@ __v4l2_find_nearest_size(const void *array, size_t array_size,
 }
 EXPORT_SYMBOL_GPL(__v4l2_find_nearest_size);
 
+#define FRACT_CMP(a, OP, b)				\
+	((u64)(a).numerator * (b).denominator OP	\
+	 (u64)(b).numerator * (a).denominator)
+
+int v4l2_find_closest_fract(struct v4l2_fract x, const struct v4l2_fract *array,
+			    size_t num)
+{
+	int i;
+
+	for (i = 0; i < num - 1; i++) {
+		struct v4l2_fract a = array[i];
+		struct v4l2_fract b = array[i + 1];
+		struct v4l2_fract midpoint = {
+			.numerator = a.numerator * b.denominator +
+				     b.numerator * a.denominator,
+			.denominator = 2 * a.denominator * b.denominator,
+		};
+
+		if (FRACT_CMP(x, <=, midpoint))
+			break;
+	}
+
+	return i;
+}
+EXPORT_SYMBOL_GPL(v4l2_find_closest_fract);
+
 void v4l2_get_timestamp(struct timeval *tv)
 {
 	struct timespec ts;
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index cdc87ec..e388f4e 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -350,6 +350,18 @@ __v4l2_find_nearest_size(const void *array, size_t array_size,
 			 size_t height_offset, s32 width, s32 height);
 
 /**
+ * v4l2_find_closest_fract - locate the closest element in a sorted array
+ * @x: The reference value.
+ * @array: The array in which to look for the closest element. Must be sorted
+ *  in ascending order.
+ * @num: number of elements in 'array'.
+ *
+ * Returns the index of the element closest to 'x'.
+ */
+int v4l2_find_closest_fract(struct v4l2_fract x, const struct v4l2_fract *array,
+			    size_t num);
+
+/**
  * v4l2_get_timestamp - helper routine to get a timestamp to be used when
  *	filling streaming metadata. Internally, it uses ktime_get_ts(),
  *	which is the recommended way to get it.
-- 
2.7.4

[PATCH 4/5] 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>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/media/i2c/video-i2c.c | 76 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 64 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index 90d389b..916f36e 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);
 
@@ -98,6 +104,22 @@ static int amg88xx_xfer(struct video_i2c_data *data, char *buf)
 				data->chip->buffer_size);
 }
 
+#define AMG88XX_REG_FPSC		0x02
+#define AMG88XX_FPSC_1FPS		BIT(0)
+
+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[] = {
@@ -172,14 +194,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,
 	},
@@ -244,7 +273,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();
 
@@ -306,19 +336,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)
@@ -425,15 +462,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;
 }
@@ -478,12 +514,26 @@ 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 n;
+
+	n = v4l2_find_closest_fract(parm->parm.capture.timeperframe,
+				    data->chip->frame_intervals,
+				    data->chip->num_frame_intervals);
+
+	data->frame_interval = data->chip->frame_intervals[n];
+
+	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,
@@ -495,7 +545,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,
@@ -572,6 +622,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 5/5] 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>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/media/i2c/video-i2c.c | 140 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 138 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index 916f36e..93822f4 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,6 +95,9 @@ 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);
 };
@@ -104,6 +108,14 @@ static int amg88xx_xfer(struct video_i2c_data *data, char *buf)
 				data->chip->buffer_size);
 }
 
+#define AMG88XX_REG_PCTL		0x00
+#define AMG88XX_PCTL_NORMAL		0x00
+#define AMG88XX_PCTL_SLEEP		0x10
+
+#define AMG88XX_REG_RST			0x01
+#define AMG88XX_RST_FLAG		0x30
+#define AMG88XX_RST_INIT		0x3f
+
 #define AMG88XX_REG_FPSC		0x02
 #define AMG88XX_FPSC_1FPS		BIT(0)
 
@@ -120,6 +132,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[] = {
@@ -151,7 +216,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, 0x0e, &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;
 
@@ -210,6 +283,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,
 	},
 };
@@ -336,14 +410,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,
@@ -352,6 +433,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);
 
@@ -367,6 +451,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);
 }
@@ -627,6 +713,18 @@ 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);
+	pm_runtime_mark_last_busy(&client->dev);
+	pm_runtime_put_autosuspend(&client->dev);
+
 	if (data->chip->hwmon_init) {
 		ret = data->chip->hwmon_init(data);
 		if (ret < 0) {
@@ -637,10 +735,17 @@ 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;
 
 	return 0;
 
+error_pm_disable:
+	pm_runtime_get_sync(&client->dev);
+	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);
@@ -654,6 +759,13 @@ static int video_i2c_remove(struct i2c_client *client)
 	struct video_i2c_data *data = i2c_get_clientdata(client);
 
 	video_unregister_device(&data->vdev);
+
+	pm_runtime_get_sync(&client->dev);
+	pm_runtime_disable(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+	pm_runtime_put_noidle(&client->dev);
+	data->chip->set_power(data, false);
+
 	v4l2_device_unregister(&data->v4l2_dev);
 
 	mutex_destroy(&data->lock);
@@ -662,6 +774,29 @@ static int video_i2c_remove(struct i2c_client *client)
 	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 },
 	{}
@@ -678,6 +813,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 1/5] media: video-i2c: avoid accessing released memory area when removing driver

[Matt Ranostay]

On Mon, Sep 17, 2018 at 9:03 AM Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
> The struct video_i2c_data is released when video_unregister_device() is
> called, but it will still be accessed after calling
> video_unregister_device().
>
> Use devm_kzalloc() and let the memory be automatically released on driver
> detach.
>
> 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>

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

> ---
>  drivers/media/i2c/video-i2c.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index 06d29d8..b7a2af9 100644
> --- a/drivers/media/i2c/video-i2c.c
> +++ b/drivers/media/i2c/video-i2c.c
> @@ -508,20 +508,15 @@ static const struct v4l2_ioctl_ops video_i2c_ioctl_ops = {
>         .vidioc_streamoff               = vb2_ioctl_streamoff,
>  };
>
> -static void video_i2c_release(struct video_device *vdev)
> -{
> -       kfree(video_get_drvdata(vdev));
> -}
> -
>  static int video_i2c_probe(struct i2c_client *client,
>                              const struct i2c_device_id *id)
>  {
>         struct video_i2c_data *data;
>         struct v4l2_device *v4l2_dev;
>         struct vb2_queue *queue;
> -       int ret = -ENODEV;
> +       int ret;
>
> -       data = kzalloc(sizeof(*data), GFP_KERNEL);
> +       data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
>         if (!data)
>                 return -ENOMEM;
>
> @@ -530,7 +525,7 @@ static int video_i2c_probe(struct i2c_client *client,
>         else if (id)
>                 data->chip = &video_i2c_chip[id->driver_data];
>         else
> -               goto error_free_device;
> +               return -ENODEV;
>
>         data->client = client;
>         v4l2_dev = &data->v4l2_dev;
> @@ -538,7 +533,7 @@ static int video_i2c_probe(struct i2c_client *client,
>
>         ret = v4l2_device_register(&client->dev, v4l2_dev);
>         if (ret < 0)
> -               goto error_free_device;
> +               return ret;
>
>         mutex_init(&data->lock);
>         mutex_init(&data->queue_lock);
> @@ -568,7 +563,7 @@ static int video_i2c_probe(struct i2c_client *client,
>         data->vdev.fops = &video_i2c_fops;
>         data->vdev.lock = &data->lock;
>         data->vdev.ioctl_ops = &video_i2c_ioctl_ops;
> -       data->vdev.release = video_i2c_release;
> +       data->vdev.release = video_device_release_empty;
>         data->vdev.device_caps = V4L2_CAP_VIDEO_CAPTURE |
>                                  V4L2_CAP_READWRITE | V4L2_CAP_STREAMING;
>
> @@ -597,9 +592,6 @@ static int video_i2c_probe(struct i2c_client *client,
>         mutex_destroy(&data->lock);
>         mutex_destroy(&data->queue_lock);
>
> -error_free_device:
> -       kfree(data);
> -
>         return ret;
>  }
>
> --
> 2.7.4
>

Re: [PATCH 2/5] media: video-i2c: use i2c regmap

[Matt Ranostay]

On Mon, Sep 17, 2018 at 9:03 AM Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
> 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>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  drivers/media/i2c/video-i2c.c | 54 ++++++++++++++++++++++---------------------
>  1 file changed, 28 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index b7a2af9..90d389b 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);
>
> @@ -85,24 +94,8 @@ struct video_i2c_chip {
>
>  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;
> -
> -       msg[1].addr = client->addr;
> -       msg[1].flags = I2C_M_RD;
> -       msg[1].len = data->chip->buffer_size;
> -       msg[1].buf = (char *)buf;
> -
> -       ret = i2c_transfer(client->adapter, msg, 2);
> -
> -       return (ret == 2) ? 0 : -EIO;
> +       return regmap_bulk_read(data->regmap, 0x80, buf,
> +                               data->chip->buffer_size);

May as well make 0x80 into a AMG88XX_REG_* define as in the later
patch in this series

>  }
>
>  #if IS_ENABLED(CONFIG_HWMON)
> @@ -133,12 +126,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, 0x0e, &buf, 2);

Same with 0x0e

- Matt

> +       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 +160,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 +179,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 +348,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));
> @@ -527,7 +526,10 @@ static int video_i2c_probe(struct i2c_client *client,
>         else
>                 return -ENODEV;
>
> -       data->client = client;
> +       data->regmap = devm_regmap_init_i2c(client, data->chip->regmap_config);
> +       if (IS_ERR(data->regmap))
> +               return PTR_ERR(data->regmap);
> +
>         v4l2_dev = &data->v4l2_dev;
>         strlcpy(v4l2_dev->name, VIDEO_I2C_DRIVER, sizeof(v4l2_dev->name));
>
> --
> 2.7.4
>

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

[Matt Ranostay]

On Mon, Sep 17, 2018 at 6:03 PM Akinobu Mita <akinobu.mita@gmail.com> 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>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  drivers/media/i2c/video-i2c.c | 140 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 138 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index 916f36e..93822f4 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,6 +95,9 @@ 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);
>  };
> @@ -104,6 +108,14 @@ static int amg88xx_xfer(struct video_i2c_data *data, char *buf)
>                                 data->chip->buffer_size);
>  }
>
> +#define AMG88XX_REG_PCTL               0x00
> +#define AMG88XX_PCTL_NORMAL            0x00
> +#define AMG88XX_PCTL_SLEEP             0x10
> +
> +#define AMG88XX_REG_RST                        0x01
> +#define AMG88XX_RST_FLAG               0x30
> +#define AMG88XX_RST_INIT               0x3f
> +
>  #define AMG88XX_REG_FPSC               0x02
>  #define AMG88XX_FPSC_1FPS              BIT(0)
>
> @@ -120,6 +132,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[] = {
> @@ -151,7 +216,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, 0x0e, &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;
>
> @@ -210,6 +283,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,
>         },
>  };
> @@ -336,14 +410,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,
> @@ -352,6 +433,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);
>
> @@ -367,6 +451,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);
>  }
> @@ -627,6 +713,18 @@ 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);

2 seconds arbitrary for runtime suspending? I could be blind (or that
I can't read the part English + Japanese datasheet) but not sure how
much power is saved in low-power mode.

In any case looks fine to me.

- Matt

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

> +       pm_runtime_use_autosuspend(&client->dev);
> +       pm_runtime_mark_last_busy(&client->dev);
> +       pm_runtime_put_autosuspend(&client->dev);
> +
>         if (data->chip->hwmon_init) {
>                 ret = data->chip->hwmon_init(data);
>                 if (ret < 0) {
> @@ -637,10 +735,17 @@ 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;
>
>         return 0;
>
> +error_pm_disable:
> +       pm_runtime_get_sync(&client->dev);
> +       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);
> @@ -654,6 +759,13 @@ static int video_i2c_remove(struct i2c_client *client)
>         struct video_i2c_data *data = i2c_get_clientdata(client);
>
>         video_unregister_device(&data->vdev);
> +
> +       pm_runtime_get_sync(&client->dev);
> +       pm_runtime_disable(&client->dev);
> +       pm_runtime_set_suspended(&client->dev);
> +       pm_runtime_put_noidle(&client->dev);
> +       data->chip->set_power(data, false);
> +
>         v4l2_device_unregister(&data->v4l2_dev);
>
>         mutex_destroy(&data->lock);
> @@ -662,6 +774,29 @@ static int video_i2c_remove(struct i2c_client *client)
>         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 },
>         {}
> @@ -678,6 +813,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 4/5] media: video-i2c: support changing frame interval

[Matt Ranostay]

On Mon, Sep 17, 2018 at 6:03 PM Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
> 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>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

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

> ---
>  drivers/media/i2c/video-i2c.c | 76 ++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 64 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index 90d389b..916f36e 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);
>
> @@ -98,6 +104,22 @@ static int amg88xx_xfer(struct video_i2c_data *data, char *buf)
>                                 data->chip->buffer_size);
>  }
>
> +#define AMG88XX_REG_FPSC               0x02
> +#define AMG88XX_FPSC_1FPS              BIT(0)
> +
> +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[] = {
> @@ -172,14 +194,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,
>         },
> @@ -244,7 +273,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();
>
> @@ -306,19 +336,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)
> @@ -425,15 +462,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;
>  }
> @@ -478,12 +514,26 @@ 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 n;
> +
> +       n = v4l2_find_closest_fract(parm->parm.capture.timeperframe,
> +                                   data->chip->frame_intervals,
> +                                   data->chip->num_frame_intervals);
> +
> +       data->frame_interval = data->chip->frame_intervals[n];
> +
> +       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,
> @@ -495,7 +545,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,
> @@ -572,6 +622,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
>

Re: [PATCH 2/5] media: video-i2c: use i2c regmap

[Akinobu Mita]

2018年9月18日(火) 12:19 Matt Ranostay <matt.ranostay@konsulko.com>:
>
> On Mon, Sep 17, 2018 at 9:03 AM Akinobu Mita <akinobu.mita@gmail.com> wrote:
> >
> > 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>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > ---
> >  drivers/media/i2c/video-i2c.c | 54 ++++++++++++++++++++++---------------------
> >  1 file changed, 28 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> > index b7a2af9..90d389b 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);
> >
> > @@ -85,24 +94,8 @@ struct video_i2c_chip {
> >
> >  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;
> > -
> > -       msg[1].addr = client->addr;
> > -       msg[1].flags = I2C_M_RD;
> > -       msg[1].len = data->chip->buffer_size;
> > -       msg[1].buf = (char *)buf;
> > -
> > -       ret = i2c_transfer(client->adapter, msg, 2);
> > -
> > -       return (ret == 2) ? 0 : -EIO;
> > +       return regmap_bulk_read(data->regmap, 0x80, buf,
> > +                               data->chip->buffer_size);
>
> May as well make 0x80 into a AMG88XX_REG_* define as in the later
> patch in this series

Sounds good. I'll do in v2.

> >  }
> >
> >  #if IS_ENABLED(CONFIG_HWMON)
> > @@ -133,12 +126,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, 0x0e, &buf, 2);
>
> Same with 0x0e

OK.

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

[Akinobu Mita]

2018年9月18日(火) 15:23 Matt Ranostay <matt.ranostay@konsulko.com>:
>
> On Mon, Sep 17, 2018 at 6:03 PM Akinobu Mita <akinobu.mita@gmail.com> 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>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > ---
> >  drivers/media/i2c/video-i2c.c | 140 +++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 138 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> > index 916f36e..93822f4 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,6 +95,9 @@ 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);
> >  };
> > @@ -104,6 +108,14 @@ static int amg88xx_xfer(struct video_i2c_data *data, char *buf)
> >                                 data->chip->buffer_size);
> >  }
> >
> > +#define AMG88XX_REG_PCTL               0x00
> > +#define AMG88XX_PCTL_NORMAL            0x00
> > +#define AMG88XX_PCTL_SLEEP             0x10
> > +
> > +#define AMG88XX_REG_RST                        0x01
> > +#define AMG88XX_RST_FLAG               0x30
> > +#define AMG88XX_RST_INIT               0x3f
> > +
> >  #define AMG88XX_REG_FPSC               0x02
> >  #define AMG88XX_FPSC_1FPS              BIT(0)
> >
> > @@ -120,6 +132,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[] = {
> > @@ -151,7 +216,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, 0x0e, &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;
> >
> > @@ -210,6 +283,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,
> >         },
> >  };
> > @@ -336,14 +410,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,
> > @@ -352,6 +433,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);
> >
> > @@ -367,6 +451,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);
> >  }
> > @@ -627,6 +713,18 @@ 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);
>
> 2 seconds arbitrary for runtime suspending? I could be blind (or that
> I can't read the part English + Japanese datasheet) but not sure how
> much power is saved in low-power mode.

According to 4-4) Detection characteristics,

Normal mode: Typ. 4.5 mA
Sleep mode: Typ. 0.2 mA

I arbitrarily selected 2 seconds for runtime suspending as it can be configured
to any value through sysfs interface.

> In any case looks fine to me.
>
> - Matt
>
> Reviewed-by: Matt Ranostay <matt.ranostay@konsulko.com>
>
> > +       pm_runtime_use_autosuspend(&client->dev);
> > +       pm_runtime_mark_last_busy(&client->dev);
> > +       pm_runtime_put_autosuspend(&client->dev);
> > +
> >         if (data->chip->hwmon_init) {
> >                 ret = data->chip->hwmon_init(data);
> >                 if (ret < 0) {
> > @@ -637,10 +735,17 @@ 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;
> >
> >         return 0;
> >
> > +error_pm_disable:
> > +       pm_runtime_get_sync(&client->dev);
> > +       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);
> > @@ -654,6 +759,13 @@ static int video_i2c_remove(struct i2c_client *client)
> >         struct video_i2c_data *data = i2c_get_clientdata(client);
> >
> >         video_unregister_device(&data->vdev);
> > +
> > +       pm_runtime_get_sync(&client->dev);
> > +       pm_runtime_disable(&client->dev);
> > +       pm_runtime_set_suspended(&client->dev);
> > +       pm_runtime_put_noidle(&client->dev);
> > +       data->chip->set_power(data, false);
> > +
> >         v4l2_device_unregister(&data->v4l2_dev);
> >
> >         mutex_destroy(&data->lock);
> > @@ -662,6 +774,29 @@ static int video_i2c_remove(struct i2c_client *client)
> >         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 },
> >         {}
> > @@ -678,6 +813,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 1/5] media: video-i2c: avoid accessing released memory area when removing driver

[Sakari Ailus]

Hi Mita-san,

On Tue, Sep 18, 2018 at 01:03:07AM +0900, Akinobu Mita wrote:
> The struct video_i2c_data is released when video_unregister_device() is
> called, but it will still be accessed after calling
> video_unregister_device().
> 
> Use devm_kzalloc() and let the memory be automatically released on driver
> detach.
> 
> 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>
> ---
>  drivers/media/i2c/video-i2c.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index 06d29d8..b7a2af9 100644
> --- a/drivers/media/i2c/video-i2c.c
> +++ b/drivers/media/i2c/video-i2c.c
> @@ -508,20 +508,15 @@ static const struct v4l2_ioctl_ops video_i2c_ioctl_ops = {
>  	.vidioc_streamoff		= vb2_ioctl_streamoff,
>  };
>  
> -static void video_i2c_release(struct video_device *vdev)
> -{
> -	kfree(video_get_drvdata(vdev));

This is actually correct: it ensures that that the device data stays in
place as long as the device is being accessed. Allocating device data with
devm_kzalloc() no longer guarantees that, and is not the right thing to do
for that reason.

> -}
> -
>  static int video_i2c_probe(struct i2c_client *client,
>  			     const struct i2c_device_id *id)
>  {
>  	struct video_i2c_data *data;
>  	struct v4l2_device *v4l2_dev;
>  	struct vb2_queue *queue;
> -	int ret = -ENODEV;
> +	int ret;
>  
> -	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  
> @@ -530,7 +525,7 @@ static int video_i2c_probe(struct i2c_client *client,
>  	else if (id)
>  		data->chip = &video_i2c_chip[id->driver_data];
>  	else
> -		goto error_free_device;
> +		return -ENODEV;
>  
>  	data->client = client;
>  	v4l2_dev = &data->v4l2_dev;
> @@ -538,7 +533,7 @@ static int video_i2c_probe(struct i2c_client *client,
>  
>  	ret = v4l2_device_register(&client->dev, v4l2_dev);
>  	if (ret < 0)
> -		goto error_free_device;
> +		return ret;
>  
>  	mutex_init(&data->lock);
>  	mutex_init(&data->queue_lock);
> @@ -568,7 +563,7 @@ static int video_i2c_probe(struct i2c_client *client,
>  	data->vdev.fops = &video_i2c_fops;
>  	data->vdev.lock = &data->lock;
>  	data->vdev.ioctl_ops = &video_i2c_ioctl_ops;
> -	data->vdev.release = video_i2c_release;
> +	data->vdev.release = video_device_release_empty;
>  	data->vdev.device_caps = V4L2_CAP_VIDEO_CAPTURE |
>  				 V4L2_CAP_READWRITE | V4L2_CAP_STREAMING;
>  
> @@ -597,9 +592,6 @@ static int video_i2c_probe(struct i2c_client *client,
>  	mutex_destroy(&data->lock);
>  	mutex_destroy(&data->queue_lock);
>  
> -error_free_device:
> -	kfree(data);
> -
>  	return ret;
>  }
>  

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 3/5] media: v4l2-common: add v4l2_find_closest_fract()

[Sakari Ailus]

Hi Mita-san,

On Tue, Sep 18, 2018 at 01:03:09AM +0900, Akinobu Mita wrote:
> Add a function to locate the closest element in a sorted v4l2_fract array.
> 
> The implementation is based on find_closest() macro in linux/util_macros.h
> and the way to compare two v4l2_fract in vivid_vid_cap_s_parm in
> drivers/media/platform/vivid/vivid-vid-cap.c.
> 
> 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>
> ---
>  drivers/media/v4l2-core/v4l2-common.c | 26 ++++++++++++++++++++++++++
>  include/media/v4l2-common.h           | 12 ++++++++++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index b518b92..91bd460 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -387,6 +387,32 @@ __v4l2_find_nearest_size(const void *array, size_t array_size,
>  }
>  EXPORT_SYMBOL_GPL(__v4l2_find_nearest_size);
>  
> +#define FRACT_CMP(a, OP, b)				\
> +	((u64)(a).numerator * (b).denominator OP	\
> +	 (u64)(b).numerator * (a).denominator)
> +
> +int v4l2_find_closest_fract(struct v4l2_fract x, const struct v4l2_fract *array,

unsigned int ?

> +			    size_t num)
> +{
> +	int i;
> +
> +	for (i = 0; i < num - 1; i++) {
> +		struct v4l2_fract a = array[i];
> +		struct v4l2_fract b = array[i + 1];
> +		struct v4l2_fract midpoint = {
> +			.numerator = a.numerator * b.denominator +
> +				     b.numerator * a.denominator,

Assuming the entire range could be in use, this may lead to an overflow.
Same on the line below.

I also wonder if e.g. a binary search would be more effective than going
through the entire list.

> +			.denominator = 2 * a.denominator * b.denominator,
> +		};
> +
> +		if (FRACT_CMP(x, <=, midpoint))
> +			break;
> +	}
> +
> +	return i;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_find_closest_fract);
> +
>  void v4l2_get_timestamp(struct timeval *tv)
>  {
>  	struct timespec ts;
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index cdc87ec..e388f4e 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -350,6 +350,18 @@ __v4l2_find_nearest_size(const void *array, size_t array_size,
>  			 size_t height_offset, s32 width, s32 height);
>  
>  /**
> + * v4l2_find_closest_fract - locate the closest element in a sorted array
> + * @x: The reference value.
> + * @array: The array in which to look for the closest element. Must be sorted
> + *  in ascending order.
> + * @num: number of elements in 'array'.
> + *
> + * Returns the index of the element closest to 'x'.
> + */
> +int v4l2_find_closest_fract(struct v4l2_fract x, const struct v4l2_fract *array,
> +			    size_t num);
> +
> +/**
>   * v4l2_get_timestamp - helper routine to get a timestamp to be used when
>   *	filling streaming metadata. Internally, it uses ktime_get_ts(),
>   *	which is the recommended way to get it.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

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

[Akinobu Mita]

2018年9月19日(水) 19:35 Sakari Ailus <sakari.ailus@linux.intel.com>:
>
> Hi Mita-san,
>
> On Tue, Sep 18, 2018 at 01:03:07AM +0900, Akinobu Mita wrote:
> > The struct video_i2c_data is released when video_unregister_device() is
> > called, but it will still be accessed after calling
> > video_unregister_device().
> >
> > Use devm_kzalloc() and let the memory be automatically released on driver
> > detach.
> >
> > 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>
> > ---
> >  drivers/media/i2c/video-i2c.c | 18 +++++-------------
> >  1 file changed, 5 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> > index 06d29d8..b7a2af9 100644
> > --- a/drivers/media/i2c/video-i2c.c
> > +++ b/drivers/media/i2c/video-i2c.c
> > @@ -508,20 +508,15 @@ static const struct v4l2_ioctl_ops video_i2c_ioctl_ops = {
> >       .vidioc_streamoff               = vb2_ioctl_streamoff,
> >  };
> >
> > -static void video_i2c_release(struct video_device *vdev)
> > -{
> > -     kfree(video_get_drvdata(vdev));
>
> This is actually correct: it ensures that that the device data stays in
> place as long as the device is being accessed. Allocating device data with
> devm_kzalloc() no longer guarantees that, and is not the right thing to do
> for that reason.

I have actually inserted printk() each line in video_i2_remove().  When
rmmod this driver, video_i2c_release() (and also kfree) is called while
executing video_unregister_device().  Because video_unregister_device()
releases the last reference to data->vdev.dev, then v4l2_device_release()
callback executes data->vdev.release.

So use after freeing video_i2c_data actually happened.

In this patch, devm_kzalloc() is called with client->dev (not with vdev->dev).
So the allocated memory is released when the last user of client->dev
is gone (maybe just after video_i2_remove() is finished).

> > -}
> > -
> >  static int video_i2c_probe(struct i2c_client *client,
> >                            const struct i2c_device_id *id)
> >  {
> >       struct video_i2c_data *data;
> >       struct v4l2_device *v4l2_dev;
> >       struct vb2_queue *queue;
> > -     int ret = -ENODEV;
> > +     int ret;
> >
> > -     data = kzalloc(sizeof(*data), GFP_KERNEL);
> > +     data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> >       if (!data)
> >               return -ENOMEM;
> >
> > @@ -530,7 +525,7 @@ static int video_i2c_probe(struct i2c_client *client,
> >       else if (id)
> >               data->chip = &video_i2c_chip[id->driver_data];
> >       else
> > -             goto error_free_device;
> > +             return -ENODEV;
> >
> >       data->client = client;
> >       v4l2_dev = &data->v4l2_dev;
> > @@ -538,7 +533,7 @@ static int video_i2c_probe(struct i2c_client *client,
> >
> >       ret = v4l2_device_register(&client->dev, v4l2_dev);
> >       if (ret < 0)
> > -             goto error_free_device;
> > +             return ret;
> >
> >       mutex_init(&data->lock);
> >       mutex_init(&data->queue_lock);
> > @@ -568,7 +563,7 @@ static int video_i2c_probe(struct i2c_client *client,
> >       data->vdev.fops = &video_i2c_fops;
> >       data->vdev.lock = &data->lock;
> >       data->vdev.ioctl_ops = &video_i2c_ioctl_ops;
> > -     data->vdev.release = video_i2c_release;
> > +     data->vdev.release = video_device_release_empty;
> >       data->vdev.device_caps = V4L2_CAP_VIDEO_CAPTURE |
> >                                V4L2_CAP_READWRITE | V4L2_CAP_STREAMING;
> >
> > @@ -597,9 +592,6 @@ static int video_i2c_probe(struct i2c_client *client,
> >       mutex_destroy(&data->lock);
> >       mutex_destroy(&data->queue_lock);
> >
> > -error_free_device:
> > -     kfree(data);
> > -
> >       return ret;
> >  }
> >
>
> --
> Regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com

Re: [PATCH 3/5] media: v4l2-common: add v4l2_find_closest_fract()

[Akinobu Mita]

2018年9月19日(水) 20:18 Sakari Ailus <sakari.ailus@linux.intel.com>:
>
> Hi Mita-san,
>
> On Tue, Sep 18, 2018 at 01:03:09AM +0900, Akinobu Mita wrote:
> > Add a function to locate the closest element in a sorted v4l2_fract array.
> >
> > The implementation is based on find_closest() macro in linux/util_macros.h
> > and the way to compare two v4l2_fract in vivid_vid_cap_s_parm in
> > drivers/media/platform/vivid/vivid-vid-cap.c.
> >
> > 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>
> > ---
> >  drivers/media/v4l2-core/v4l2-common.c | 26 ++++++++++++++++++++++++++
> >  include/media/v4l2-common.h           | 12 ++++++++++++
> >  2 files changed, 38 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > index b518b92..91bd460 100644
> > --- a/drivers/media/v4l2-core/v4l2-common.c
> > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > @@ -387,6 +387,32 @@ __v4l2_find_nearest_size(const void *array, size_t array_size,
> >  }
> >  EXPORT_SYMBOL_GPL(__v4l2_find_nearest_size);
> >
> > +#define FRACT_CMP(a, OP, b)                          \
> > +     ((u64)(a).numerator * (b).denominator OP        \
> > +      (u64)(b).numerator * (a).denominator)
> > +
> > +int v4l2_find_closest_fract(struct v4l2_fract x, const struct v4l2_fract *array,
>
> unsigned int ?

As you noticed below, this function may lead to an overflow.  So I planned
to make it return -EOVERFLOW with help of linux/overflow.h.

But now I'm start thinking that finding closest (rounding) value is
overkill.  Instead finding smallest (ceiling) or largest (floor) value is
enough just like in vivid_vid_cap_s_parm() in vivid-vid-cap.c and we don't
need to be bothered with overflows.

> > +                         size_t num)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < num - 1; i++) {
> > +             struct v4l2_fract a = array[i];
> > +             struct v4l2_fract b = array[i + 1];
> > +             struct v4l2_fract midpoint = {
> > +                     .numerator = a.numerator * b.denominator +
> > +                                  b.numerator * a.denominator,
>
> Assuming the entire range could be in use, this may lead to an overflow.
> Same on the line below.
>
> I also wonder if e.g. a binary search would be more effective than going
> through the entire list.

The video-i2c driver will use this function with an array of 2 objects
and the vivid driver may also use this function with an array of 5 objects.
So simple linear search is enough for now, but it can be changed to
bsearch without changing external interface if needed sometime.

> > +                     .denominator = 2 * a.denominator * b.denominator,
> > +             };
> > +
> > +             if (FRACT_CMP(x, <=, midpoint))
> > +                     break;
> > +     }
> > +
> > +     return i;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_find_closest_fract);
> > +
> >  void v4l2_get_timestamp(struct timeval *tv)
> >  {
> >       struct timespec ts;
> > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> > index cdc87ec..e388f4e 100644
> > --- a/include/media/v4l2-common.h
> > +++ b/include/media/v4l2-common.h
> > @@ -350,6 +350,18 @@ __v4l2_find_nearest_size(const void *array, size_t array_size,
> >                        size_t height_offset, s32 width, s32 height);
> >
> >  /**
> > + * v4l2_find_closest_fract - locate the closest element in a sorted array
> > + * @x: The reference value.
> > + * @array: The array in which to look for the closest element. Must be sorted
> > + *  in ascending order.
> > + * @num: number of elements in 'array'.
> > + *
> > + * Returns the index of the element closest to 'x'.
> > + */
> > +int v4l2_find_closest_fract(struct v4l2_fract x, const struct v4l2_fract *array,
> > +                         size_t num);
> > +
> > +/**
> >   * v4l2_get_timestamp - helper routine to get a timestamp to be used when
> >   *   filling streaming metadata. Internally, it uses ktime_get_ts(),
> >   *   which is the recommended way to get it.
>
> --
> Regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com