Re: [PATCH 06/12] media: mt9m001: switch s_power callback to runtime PM

[Akinobu Mita <akinobu.mita@gmail.com>]

2019年1月7日(月) 19:00 Sakari Ailus <sakari.ailus@linux.intel.com>:
>
> Hi Mita-san,
>
> Thanks for the patchset.
>
> On Sun, Dec 23, 2018 at 02:12:48AM +0900, Akinobu Mita wrote:
> > Switch s_power() callback to runtime PM framework.  This also removes
> > soc_camera specific power management code and introduces reset and standby
> > gpios instead.
> >
> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > ---
> >  drivers/media/i2c/mt9m001.c | 242 ++++++++++++++++++++++++++++++++------------
> >  1 file changed, 178 insertions(+), 64 deletions(-)
> >
> > diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
> > index c0180fdc..f20188a 100644
> > --- a/drivers/media/i2c/mt9m001.c
> > +++ b/drivers/media/i2c/mt9m001.c
> > @@ -5,6 +5,10 @@
> >   * Copyright (C) 2008, Guennadi Liakhovetski <kernel@pengutronix.de>
> >   */
> >
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/videodev2.h>
> >  #include <linux/slab.h>
> >  #include <linux/i2c.h>
> > @@ -13,7 +17,6 @@
> >
> >  #include <media/soc_camera.h>
> >  #include <media/drv-intf/soc_mediabus.h>
> > -#include <media/v4l2-clk.h>
> >  #include <media/v4l2-subdev.h>
> >  #include <media/v4l2-ctrls.h>
> >
> > @@ -92,8 +95,12 @@ struct mt9m001 {
> >               struct v4l2_ctrl *autoexposure;
> >               struct v4l2_ctrl *exposure;
> >       };
> > +     bool streaming;
> > +     struct mutex mutex;
> >       struct v4l2_rect rect;  /* Sensor window */
> > -     struct v4l2_clk *clk;
> > +     struct clk *clk;
> > +     struct gpio_desc *standby_gpio;
> > +     struct gpio_desc *reset_gpio;
> >       const struct mt9m001_datafmt *fmt;
> >       const struct mt9m001_datafmt *fmts;
> >       int num_fmts;
> > @@ -177,8 +184,7 @@ static int mt9m001_init(struct i2c_client *client)
> >       return multi_reg_write(client, init_regs, ARRAY_SIZE(init_regs));
> >  }
> >
> > -static int mt9m001_apply_selection(struct v4l2_subdev *sd,
> > -                                 struct v4l2_rect *rect)
> > +static int mt9m001_apply_selection(struct v4l2_subdev *sd)
> >  {
> >       struct i2c_client *client = v4l2_get_subdevdata(sd);
> >       struct mt9m001 *mt9m001 = to_mt9m001(client);
> > @@ -190,11 +196,11 @@ static int mt9m001_apply_selection(struct v4l2_subdev *sd,
> >                * The caller provides a supported format, as verified per
> >                * call to .set_fmt(FORMAT_TRY).
> >                */
> > -             { MT9M001_COLUMN_START, rect->left },
> > -             { MT9M001_ROW_START, rect->top },
> > -             { MT9M001_WINDOW_WIDTH, rect->width - 1 },
> > +             { MT9M001_COLUMN_START, mt9m001->rect.left },
> > +             { MT9M001_ROW_START, mt9m001->rect.top },
> > +             { MT9M001_WINDOW_WIDTH, mt9m001->rect.width - 1 },
> >               { MT9M001_WINDOW_HEIGHT,
> > -                     rect->height + mt9m001->y_skip_top - 1 },
> > +                     mt9m001->rect.height + mt9m001->y_skip_top - 1 },
> >       };
> >
> >       return multi_reg_write(client, regs, ARRAY_SIZE(regs));
> > @@ -203,11 +209,50 @@ static int mt9m001_apply_selection(struct v4l2_subdev *sd,
> >  static int mt9m001_s_stream(struct v4l2_subdev *sd, int enable)
> >  {
> >       struct i2c_client *client = v4l2_get_subdevdata(sd);
> > +     struct mt9m001 *mt9m001 = to_mt9m001(client);
> > +     int ret = 0;
> >
> > -     /* Switch to master "normal" mode or stop sensor readout */
> > -     if (reg_write(client, MT9M001_OUTPUT_CONTROL, enable ? 2 : 0) < 0)
> > -             return -EIO;
> > -     return 0;
> > +     mutex_lock(&mt9m001->mutex);
> > +
> > +     if (mt9m001->streaming == enable)
> > +             goto done;
> > +
> > +     if (enable) {
> > +             ret = pm_runtime_get_sync(&client->dev);
> > +             if (ret < 0) {
> > +                     pm_runtime_put_noidle(&client->dev);
> > +                     goto done;
>
> How about adding another label for calling pm_runtime_put()? The error
> handling is the same in all cases. You can also use pm_runtime_put()
> instead of pm_runtime_put_noidle() here; there's no harm done.

There are two ways that I can think of.  Which one do you prefer?

(1)
done:
        mutex_unlock(&mt9m001->mutex);

        return 0;

enable_error:
        pm_runtime_put(&client->dev);
        mutex_unlock(&mt9m001->mutex);

        return ret;
}

(2)
done:
        if (ret && enable)
               pm_runtime_put(&client->dev);

        mutex_unlock(&mt9m001->mutex);

        return ret;
}

> > +             }
> > +
> > +             ret = mt9m001_apply_selection(sd);
> > +             if (ret) {
> > +                     pm_runtime_put(&client->dev);
> > +                     goto done;
> > +             }
> > +
> > +             ret = __v4l2_ctrl_handler_setup(&mt9m001->hdl);
> > +             if (ret) {
> > +                     pm_runtime_put(&client->dev);
> > +                     goto done;
> > +             }
> > +
> > +             /* Switch to master "normal" mode */
> > +             ret = reg_write(client, MT9M001_OUTPUT_CONTROL, 2);
> > +             if (ret < 0) {
> > +                     pm_runtime_put(&client->dev);
> > +                     goto done;
> > +             }
> > +     } else {
> > +             /* Switch to master stop sensor readout */
> > +             reg_write(client, MT9M001_OUTPUT_CONTROL, 0);
> > +             pm_runtime_put(&client->dev);
> > +     }
> > +
> > +     mt9m001->streaming = enable;
> > +done:
> > +     mutex_unlock(&mt9m001->mutex);
> > +
> > +     return ret;
> >  }
> >
> >  static int mt9m001_set_selection(struct v4l2_subdev *sd,
> > @@ -217,7 +262,6 @@ static int mt9m001_set_selection(struct v4l2_subdev *sd,
> >       struct i2c_client *client = v4l2_get_subdevdata(sd);
> >       struct mt9m001 *mt9m001 = to_mt9m001(client);
> >       struct v4l2_rect rect = sel->r;
> > -     int ret;
> >
> >       if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE ||
> >           sel->target != V4L2_SEL_TGT_CROP)
> > @@ -243,15 +287,9 @@ static int mt9m001_set_selection(struct v4l2_subdev *sd,
> >       mt9m001->total_h = rect.height + mt9m001->y_skip_top +
> >                          MT9M001_DEFAULT_VBLANK;
> >
> > +     mt9m001->rect = rect;
> >
> > -     ret = mt9m001_apply_selection(sd, &rect);
> > -     if (!ret && v4l2_ctrl_g_ctrl(mt9m001->autoexposure) == V4L2_EXPOSURE_AUTO)
> > -             ret = reg_write(client, MT9M001_SHUTTER_WIDTH, mt9m001->total_h);
> > -
> > -     if (!ret)
> > -             mt9m001->rect = rect;
> > -
> > -     return ret;
> > +     return 0;
> >  }
> >
> >  static int mt9m001_get_selection(struct v4l2_subdev *sd,
> > @@ -395,13 +433,34 @@ static int mt9m001_s_register(struct v4l2_subdev *sd,
> >  }
> >  #endif
> >
> > -static int mt9m001_s_power(struct v4l2_subdev *sd, int on)
> > +static int mt9m001_power_on(struct mt9m001 *mt9m001)
> >  {
> > -     struct i2c_client *client = v4l2_get_subdevdata(sd);
> > -     struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
> > -     struct mt9m001 *mt9m001 = to_mt9m001(client);
> > +     int ret = clk_prepare_enable(mt9m001->clk);
> > +
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (mt9m001->standby_gpio) {
> > +             gpiod_set_value_cansleep(mt9m001->standby_gpio, 0);
> > +             usleep_range(1000, 2000);
> > +     }
> > +
> > +     if (mt9m001->reset_gpio) {
> > +             gpiod_set_value_cansleep(mt9m001->reset_gpio, 1);
> > +             usleep_range(1000, 2000);
> > +             gpiod_set_value_cansleep(mt9m001->reset_gpio, 0);
> > +             usleep_range(1000, 2000);
> > +     }
> >
> > -     return soc_camera_set_power(&client->dev, ssdd, mt9m001->clk, on);
> > +     return 0;
> > +}
> > +
> > +static int mt9m001_power_off(struct mt9m001 *mt9m001)
> > +{
> > +     gpiod_set_value_cansleep(mt9m001->standby_gpio, 1);
> > +     clk_disable_unprepare(mt9m001->clk);
> > +
> > +     return 0;
> >  }
> >
> >  static int mt9m001_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
> > @@ -429,16 +488,18 @@ static int mt9m001_s_ctrl(struct v4l2_ctrl *ctrl)
> >       struct i2c_client *client = v4l2_get_subdevdata(sd);
> >       struct v4l2_ctrl *exp = mt9m001->exposure;
> >       int data;
> > +     int ret;
> > +
> > +     if (!pm_runtime_get_if_in_use(&client->dev))
> > +             return 0;
> >
> >       switch (ctrl->id) {
> >       case V4L2_CID_VFLIP:
> >               if (ctrl->val)
> > -                     data = reg_set(client, MT9M001_READ_OPTIONS2, 0x8000);
> > +                     ret = reg_set(client, MT9M001_READ_OPTIONS2, 0x8000);
> >               else
> > -                     data = reg_clear(client, MT9M001_READ_OPTIONS2, 0x8000);
> > -             if (data < 0)
> > -                     return -EIO;
> > -             return 0;
> > +                     ret = reg_clear(client, MT9M001_READ_OPTIONS2, 0x8000);
> > +             break;
> >
> >       case V4L2_CID_GAIN:
> >               /* See Datasheet Table 7, Gain settings. */
> > @@ -448,9 +509,7 @@ static int mt9m001_s_ctrl(struct v4l2_ctrl *ctrl)
> >                       data = ((ctrl->val - (s32)ctrl->minimum) * 8 + range / 2) / range;
> >
> >                       dev_dbg(&client->dev, "Setting gain %d\n", data);
> > -                     data = reg_write(client, MT9M001_GLOBAL_GAIN, data);
> > -                     if (data < 0)
> > -                             return -EIO;
> > +                     ret = reg_write(client, MT9M001_GLOBAL_GAIN, data);
> >               } else {
> >                       /* Pack it into 1.125..15 variable step, register values 9..67 */
> >                       /* We assume qctrl->maximum - qctrl->default_value - 1 > 0 */
> > @@ -467,11 +526,9 @@ static int mt9m001_s_ctrl(struct v4l2_ctrl *ctrl)
> >
> >                       dev_dbg(&client->dev, "Setting gain from %d to %d\n",
> >                                reg_read(client, MT9M001_GLOBAL_GAIN), data);
> > -                     data = reg_write(client, MT9M001_GLOBAL_GAIN, data);
> > -                     if (data < 0)
> > -                             return -EIO;
> > +                     ret = reg_write(client, MT9M001_GLOBAL_GAIN, data);
> >               }
> > -             return 0;
> > +             break;
> >
> >       case V4L2_CID_EXPOSURE_AUTO:
> >               if (ctrl->val == V4L2_EXPOSURE_MANUAL) {
> > @@ -482,19 +539,22 @@ static int mt9m001_s_ctrl(struct v4l2_ctrl *ctrl)
> >                       dev_dbg(&client->dev,
> >                               "Setting shutter width from %d to %lu\n",
> >                               reg_read(client, MT9M001_SHUTTER_WIDTH), shutter);
> > -                     if (reg_write(client, MT9M001_SHUTTER_WIDTH, shutter) < 0)
> > -                             return -EIO;
> > +                     ret = reg_write(client, MT9M001_SHUTTER_WIDTH, shutter);
> >               } else {
> > -                     const u16 vblank = 25;
> > -
> >                       mt9m001->total_h = mt9m001->rect.height +
> > -                             mt9m001->y_skip_top + vblank;
> > -                     if (reg_write(client, MT9M001_SHUTTER_WIDTH, mt9m001->total_h) < 0)
> > -                             return -EIO;
> > +                             mt9m001->y_skip_top + MT9M001_DEFAULT_VBLANK;
> > +                     ret = reg_write(client, MT9M001_SHUTTER_WIDTH,
> > +                                     mt9m001->total_h);
> >               }
> > -             return 0;
> > +             break;
> > +     default:
> > +             ret = -EINVAL;
> > +             break;
> >       }
> > -     return -EINVAL;
> > +
> > +     pm_runtime_put(&client->dev);
> > +
> > +     return ret;
> >  }
> >
> >  /*
> > @@ -509,10 +569,6 @@ static int mt9m001_video_probe(struct soc_camera_subdev_desc *ssdd,
> >       unsigned long flags;
> >       int ret;
> >
> > -     ret = mt9m001_s_power(&mt9m001->subdev, 1);
> > -     if (ret < 0)
> > -             return ret;
> > -
> >       /* Enable the chip */
> >       data = reg_write(client, MT9M001_CHIP_ENABLE, 1);
> >       dev_dbg(&client->dev, "write: %d\n", data);
> > @@ -571,7 +627,6 @@ static int mt9m001_video_probe(struct soc_camera_subdev_desc *ssdd,
> >       ret = v4l2_ctrl_handler_setup(&mt9m001->hdl);
> >
> >  done:
> > -     mt9m001_s_power(&mt9m001->subdev, 0);
> >       return ret;
> >  }
> >
> > @@ -601,7 +656,6 @@ static const struct v4l2_subdev_core_ops mt9m001_subdev_core_ops = {
> >       .g_register     = mt9m001_g_register,
> >       .s_register     = mt9m001_s_register,
> >  #endif
> > -     .s_power        = mt9m001_s_power,
> >  };
> >
> >  static int mt9m001_enum_mbus_code(struct v4l2_subdev *sd,
> > @@ -700,6 +754,20 @@ static int mt9m001_probe(struct i2c_client *client,
> >       if (!mt9m001)
> >               return -ENOMEM;
> >
> > +     mt9m001->clk = devm_clk_get(&client->dev, NULL);
> > +     if (IS_ERR(mt9m001->clk))
> > +             return PTR_ERR(mt9m001->clk);
> > +
> > +     mt9m001->standby_gpio = devm_gpiod_get_optional(&client->dev, "standby",
> > +                                                     GPIOD_OUT_LOW);
> > +     if (IS_ERR(mt9m001->standby_gpio))
> > +             return PTR_ERR(mt9m001->standby_gpio);
> > +
> > +     mt9m001->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> > +                                                   GPIOD_OUT_LOW);
> > +     if (IS_ERR(mt9m001->reset_gpio))
> > +             return PTR_ERR(mt9m001->reset_gpio);
> > +
> >       v4l2_i2c_subdev_init(&mt9m001->subdev, client, &mt9m001_subdev_ops);
> >       v4l2_ctrl_handler_init(&mt9m001->hdl, 4);
> >       v4l2_ctrl_new_std(&mt9m001->hdl, &mt9m001_ctrl_ops,
> > @@ -722,6 +790,9 @@ static int mt9m001_probe(struct i2c_client *client,
> >       v4l2_ctrl_auto_cluster(2, &mt9m001->autoexposure,
> >                                       V4L2_EXPOSURE_MANUAL, true);
> >
> > +     mutex_init(&mt9m001->mutex);
> > +     mt9m001->hdl.lock = &mt9m001->mutex;
> > +
> >       /* Second stage probe - when a capture adapter is there */
> >       mt9m001->y_skip_top     = 0;
> >       mt9m001->rect.left      = MT9M001_COLUMN_SKIP;
> > @@ -729,18 +800,30 @@ static int mt9m001_probe(struct i2c_client *client,
> >       mt9m001->rect.width     = MT9M001_MAX_WIDTH;
> >       mt9m001->rect.height    = MT9M001_MAX_HEIGHT;
> >
> > -     mt9m001->clk = v4l2_clk_get(&client->dev, "mclk");
> > -     if (IS_ERR(mt9m001->clk)) {
> > -             ret = PTR_ERR(mt9m001->clk);
> > -             goto eclkget;
> > -     }
> > +     ret = mt9m001_power_on(mt9m001);
> > +     if (ret)
> > +             goto error_hdl_free;
> > +
> > +     pm_runtime_get_noresume(&client->dev);
> > +     pm_runtime_set_active(&client->dev);
> > +     pm_runtime_enable(&client->dev);
>
> You could replace ...get_noresume() + ...put_sync() below with a single
> pm_runtime_idle() call.

OK.

> >
> >       ret = mt9m001_video_probe(ssdd, client);
> > -     if (ret) {
> > -             v4l2_clk_put(mt9m001->clk);
> > -eclkget:
> > -             v4l2_ctrl_handler_free(&mt9m001->hdl);
> > -     }
> > +     if (ret)
> > +             goto error_power_off;
> > +
> > +     pm_runtime_put_sync(&client->dev);
> > +
> > +     return 0;
> > +
> > +error_power_off:
> > +     pm_runtime_disable(&client->dev);
> > +     pm_runtime_set_suspended(&client->dev);
> > +     pm_runtime_put_noidle(&client->dev);
> > +     mt9m001_power_off(mt9m001);
>
> A newline would be nice here.

OK.

> > +error_hdl_free:
> > +     v4l2_ctrl_handler_free(&mt9m001->hdl);
> > +     mutex_destroy(&mt9m001->mutex);
> >
> >       return ret;
> >  }
> > @@ -750,10 +833,17 @@ static int mt9m001_remove(struct i2c_client *client)
> >       struct mt9m001 *mt9m001 = to_mt9m001(client);
> >       struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
> >
> > -     v4l2_clk_put(mt9m001->clk);
> >       v4l2_device_unregister_subdev(&mt9m001->subdev);
> > +     pm_runtime_get_sync(&client->dev);
> > +
> > +     pm_runtime_disable(&client->dev);
> > +     pm_runtime_set_suspended(&client->dev);
> > +     pm_runtime_put_noidle(&client->dev);
> > +     mt9m001_power_off(mt9m001);
> > +
> >       v4l2_ctrl_handler_free(&mt9m001->hdl);
> >       mt9m001_video_remove(ssdd);
> > +     mutex_destroy(&mt9m001->mutex);
> >
> >       return 0;
> >  }
> > @@ -764,6 +854,29 @@ static const struct i2c_device_id mt9m001_id[] = {
> >  };
> >  MODULE_DEVICE_TABLE(i2c, mt9m001_id);
> >
> > +static int __maybe_unused mt9m001_runtime_resume(struct device *dev)
> > +{
> > +     struct i2c_client *client = to_i2c_client(dev);
> > +     struct mt9m001 *mt9m001 = to_mt9m001(client);
> > +
> > +     return mt9m001_power_on(mt9m001);
>
> How about changing the argument of mt9m001_power_o{n,ff} to struct device
> so you could remove these two? I think the original names are fine.

Sounds good.