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