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

[PATCH v4 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.

* v4
- Add Reviewed-by line
- Move set_power() call into release() callback

* 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: c36dbbdfa8b30b2badd4f893b59d0bd4f0bd12aa, 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 v4 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>
Reviewed-by: Matt Ranostay <matt.ranostay@konsulko.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
* v4
- Add Reviewed-by line

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

[Sakari Ailus]

On Sat, Oct 20, 2018 at 11:26:23PM +0900, Akinobu Mita 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>
> 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>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

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

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

[PATCH v4 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>
---
* v4
- No changes from v3

 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 v4 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>
---
* v4
- No changes from v3

 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

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

[Matt Ranostay]

On Sat, Oct 20, 2018 at 7:26 AM Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
> 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>
> ---
> * v4
> - No changes from v3
>
>  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)
> +

Noticed a few issues today when testing another thermal camera that
can do 0.5 fps to 64 fps with this macro..

1) This can have collision easily when numerator and denominators
multiplied have the same product, example is 0.5hz and 2hz have the
same output as 2

2) Also this doesn't reduce fractions so I am seeing 4000000 compared
with 4 for instance with a 4hz frame interval.

- Matt



>  #endif /* V4L2_COMMON_H_ */
> --
> 2.7.4
>

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

[Akinobu Mita]

2018年10月28日(日) 12:49 Matt Ranostay <matt.ranostay@konsulko.com>:
>
> On Sat, Oct 20, 2018 at 7:26 AM Akinobu Mita <akinobu.mita@gmail.com> wrote:
> >
> > 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>
> > ---
> > * v4
> > - No changes from v3
> >
> >  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)
> > +
>
> Noticed a few issues today when testing another thermal camera that
> can do 0.5 fps to 64 fps with this macro..

I expect your new thermal camera's frame_intervals will be something
like below.

static const struct v4l2_fract frame_intervals[] = {
        { 1, 64 },      /* 64 fps */
        { 1, 4 },       /* 4 fps */
        { 1, 2 },       /* 2 fps */
        { 2, 1 },       /* 0.5 fps */
};

> 1) This can have collision easily when numerator and denominators
> multiplied have the same product, example is 0.5hz and 2hz have the
> same output as 2

I think V4L2_FRACT_COMPARE() can correctly compare with 0.5hz and 2hz.

V4L2_FRACT_COMPARE({ 1, 2 }, <=, { 2, 1 }); // -->  true
V4L2_FRACT_COMPARE({ 2, 1 }, <=, { 1, 2 }); //-->  false

> 2) Also this doesn't reduce fractions so I am seeing 4000000 compared
> with 4 for instance with a 4hz frame interval.

I think this works fine, too.

V4L2_FRACT_COMPARE({ 1, 4000000 }, <=, { 1, 4 }); //-->  true
V4L2_FRACT_COMPARE({ 1, 4 }, <=, { 1, 4000000 }); //-->  false

Or, do I misunderstand your problem?

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

[Matt Ranostay]

On Sun, Oct 28, 2018 at 9:25 AM Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
> 2018年10月28日(日) 12:49 Matt Ranostay <matt.ranostay@konsulko.com>:
> >
> > On Sat, Oct 20, 2018 at 7:26 AM Akinobu Mita <akinobu.mita@gmail.com> wrote:
> > >
> > > 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>
> > > ---
> > > * v4
> > > - No changes from v3
> > >
> > >  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)
> > > +
> >
> > Noticed a few issues today when testing another thermal camera that
> > can do 0.5 fps to 64 fps with this macro..
>
> I expect your new thermal camera's frame_intervals will be something
> like below.
>
> static const struct v4l2_fract frame_intervals[] = {
>         { 1, 64 },      /* 64 fps */
>         { 1, 4 },       /* 4 fps */
>         { 1, 2 },       /* 2 fps */
>         { 2, 1 },       /* 0.5 fps */
> };
>
> > 1) This can have collision easily when numerator and denominators
> > multiplied have the same product, example is 0.5hz and 2hz have the
> > same output as 2
>
> I think V4L2_FRACT_COMPARE() can correctly compare with 0.5hz and 2hz.
>
> V4L2_FRACT_COMPARE({ 1, 2 }, <=, { 2, 1 }); // -->  true
> V4L2_FRACT_COMPARE({ 2, 1 }, <=, { 1, 2 }); //-->  false
>
> > 2) Also this doesn't reduce fractions so I am seeing 4000000 compared
> > with 4 for instance with a 4hz frame interval.
>
> I think this works fine, too.
>
> V4L2_FRACT_COMPARE({ 1, 4000000 }, <=, { 1, 4 }); //-->  true
> V4L2_FRACT_COMPARE({ 1, 4 }, <=, { 1, 4000000 }); //-->  false
>
> Or, do I misunderstand your problem?

Probably not! I didn't think of them having to be sorted in a certain
way, but will test and probably will work.

Couldn't hurt to have that noted in a comment some where as well.

- Matt

[PATCH v4 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>
---
* v4
- No changes from v3

 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 v4 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>
---
* v4
- No changes from v3

 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

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

[Matt Ranostay]

On Sat, Oct 20, 2018 at 7:26 AM 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>
> 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>
> ---
> * v4
> - No changes from v3
>
>  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++) {

Just noticed this when testing for another thermal camera.

Why is this "i < data->chip->num_frame_intervals - 1" and not just "i
< data->chip->num_frame_intervals"?
It won't ever check the last possible frame interval in the respective
array as written.

- Matt

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

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

[Akinobu Mita]

2018年10月28日(日) 12:39 Matt Ranostay <matt.ranostay@konsulko.com>:
>
> On Sat, Oct 20, 2018 at 7:26 AM 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>
> > 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>
> > ---
> > * v4
> > - No changes from v3
> >
> >  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++) {
>
> Just noticed this when testing for another thermal camera.
>
> Why is this "i < data->chip->num_frame_intervals - 1" and not just "i
> < data->chip->num_frame_intervals"?
> It won't ever check the last possible frame interval in the respective
> array as written.

It is just unnecessary to check the last entry, because the array
must be sorted in ascending order.

If it checks the last entry, the code will be more redundant like below.

for (i = 0; i < data->chip->num_frame_intervals; i++) {
        if (V4L2_FRACT_COMPARE(parm->parm.capture.timeperframe, <=,
                        data->chip->frame_intervals[i]))
                break;
}
if (i == data->chip->num_frame_intervals)
        i = data->chip->num_frame_intervals - 1;

data->frame_interval = data->chip->frame_intervals[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 v4 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>
---
* v4
- Move set_power() call into release() callback

 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..384a046 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);
 }
@@ -648,6 +736,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 +756,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 +787,40 @@ 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);
+	data->chip->set_power(data, false);
+
 	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 v4 6/6] media: video-i2c: support runtime PM

[Hans Verkuil]

On 10/20/2018 04:26 PM, 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>
> ---
> * v4
> - Move set_power() call into release() callback
> 
>  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..384a046 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);

WARNING: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
#55: FILE: drivers/media/i2c/video-i2c.c:158:
+       msleep(2);

Use usleep_range instead.

Regards,

	Hans

> +
> +	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);
>  }
> @@ -648,6 +736,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 +756,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 +787,40 @@ 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);
> +	data->chip->set_power(data, false);
> +
>  	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,
> 

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

[Hans Verkuil]

On 11/19/2018 03:26 PM, Hans Verkuil wrote:
> On 10/20/2018 04:26 PM, 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>

For the record: I've accepted patches 1-5, so no need to repost the whole series,
just this patch needs an update.

Regards,

	Hans