linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/12] media: mt9m001: switch soc_mt9m001 to a standard subdev sensor driver
@ 2018-12-22 17:12 Akinobu Mita
  2018-12-22 17:12 ` [PATCH 01/12] media: i2c: mt9m001: copy mt9m001 soc_camera " Akinobu Mita
                   ` (12 more replies)
  0 siblings, 13 replies; 31+ messages in thread
From: Akinobu Mita @ 2018-12-22 17:12 UTC (permalink / raw)
  To: linux-media, devicetree
  Cc: Akinobu Mita, Rob Herring, Guennadi Liakhovetski, Sakari Ailus,
	Mauro Carvalho Chehab

This patchset converts soc_camera mt9m001 driver to a standard subdev
sensor driver.

Akinobu Mita (12):
  media: i2c: mt9m001: copy mt9m001 soc_camera sensor driver
  media: i2c: mt9m001: dt: add binding for mt9m001
  media: mt9m001: convert to SPDX license identifer
  media: mt9m001: add of_match_table
  media: mt9m001: introduce multi_reg_write()
  media: mt9m001: switch s_power callback to runtime PM
  media: mt9m001: remove remaining soc_camera specific code
  media: mt9m001: add media controller support
  media: mt9m001: register to V4L2 asynchronous subdevice framework
  media: mt9m001: support log_status ioctl and event interface
  media: mt9m001: make VIDIOC_SUBDEV_G_FMT ioctl work with
    V4L2_SUBDEV_FORMAT_TRY
  media: mt9m001: set all mbus format field when G_FMT and S_FMT ioctls

 .../devicetree/bindings/media/i2c/mt9m001.txt      |  37 +
 drivers/media/i2c/Kconfig                          |   9 +
 drivers/media/i2c/Makefile                         |   1 +
 drivers/media/i2c/mt9m001.c                        | 900 +++++++++++++++++++++
 4 files changed, 947 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/mt9m001.txt
 create mode 100644 drivers/media/i2c/mt9m001.c

Cc: Rob Herring <robh@kernel.org>
Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
-- 
2.7.4


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 01/12] media: i2c: mt9m001: copy mt9m001 soc_camera sensor driver
  2018-12-22 17:12 [PATCH 00/12] media: mt9m001: switch soc_mt9m001 to a standard subdev sensor driver Akinobu Mita
@ 2018-12-22 17:12 ` Akinobu Mita
  2018-12-22 17:12 ` [PATCH 02/12] media: i2c: mt9m001: dt: add binding for mt9m001 Akinobu Mita
                   ` (11 subsequent siblings)
  12 siblings, 0 replies; 31+ messages in thread
From: Akinobu Mita @ 2018-12-22 17:12 UTC (permalink / raw)
  To: linux-media, devicetree
  Cc: Akinobu Mita, Guennadi Liakhovetski, Sakari Ailus, Mauro Carvalho Chehab

Copy the soc_camera based driver in v4l2 sensor driver directory.
This commit just copies the original file without modifying it.

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/Kconfig   |   7 +
 drivers/media/i2c/Makefile  |   1 +
 drivers/media/i2c/mt9m001.c | 757 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 765 insertions(+)
 create mode 100644 drivers/media/i2c/mt9m001.c

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index d933f68..0efc038 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -843,6 +843,13 @@ config VIDEO_VS6624
 	  To compile this driver as a module, choose M here: the
 	  module will be called vs6624.
 
+config VIDEO_MT9M001
+	tristate "mt9m001 support"
+	depends on SOC_CAMERA && I2C
+	help
+	  This driver supports MT9M001 cameras from Micron, monochrome
+	  and colour models.
+
 config VIDEO_MT9M032
 	tristate "MT9M032 camera sensor support"
 	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 04cbbfa..5806bd1 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -81,6 +81,7 @@ obj-$(CONFIG_VIDEO_OV7740) += ov7740.o
 obj-$(CONFIG_VIDEO_OV9650) += ov9650.o
 obj-$(CONFIG_VIDEO_OV9655) += ov9655.o
 obj-$(CONFIG_VIDEO_OV13858) += ov13858.o
+obj-$(CONFIG_VIDEO_MT9M001) += mt9m001.o
 obj-$(CONFIG_VIDEO_MT9M032) += mt9m032.o
 obj-$(CONFIG_VIDEO_MT9M111) += mt9m111.o
 obj-$(CONFIG_VIDEO_MT9P031) += mt9p031.o
diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
new file mode 100644
index 0000000..a1a85ff
--- /dev/null
+++ b/drivers/media/i2c/mt9m001.c
@@ -0,0 +1,757 @@
+/*
+ * Driver for MT9M001 CMOS Image Sensor from Micron
+ *
+ * Copyright (C) 2008, Guennadi Liakhovetski <kernel@pengutronix.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/videodev2.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/log2.h>
+#include <linux/module.h>
+
+#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>
+
+/*
+ * mt9m001 i2c address 0x5d
+ * The platform has to define struct i2c_board_info objects and link to them
+ * from struct soc_camera_host_desc
+ */
+
+/* mt9m001 selected register addresses */
+#define MT9M001_CHIP_VERSION		0x00
+#define MT9M001_ROW_START		0x01
+#define MT9M001_COLUMN_START		0x02
+#define MT9M001_WINDOW_HEIGHT		0x03
+#define MT9M001_WINDOW_WIDTH		0x04
+#define MT9M001_HORIZONTAL_BLANKING	0x05
+#define MT9M001_VERTICAL_BLANKING	0x06
+#define MT9M001_OUTPUT_CONTROL		0x07
+#define MT9M001_SHUTTER_WIDTH		0x09
+#define MT9M001_FRAME_RESTART		0x0b
+#define MT9M001_SHUTTER_DELAY		0x0c
+#define MT9M001_RESET			0x0d
+#define MT9M001_READ_OPTIONS1		0x1e
+#define MT9M001_READ_OPTIONS2		0x20
+#define MT9M001_GLOBAL_GAIN		0x35
+#define MT9M001_CHIP_ENABLE		0xF1
+
+#define MT9M001_MAX_WIDTH		1280
+#define MT9M001_MAX_HEIGHT		1024
+#define MT9M001_MIN_WIDTH		48
+#define MT9M001_MIN_HEIGHT		32
+#define MT9M001_COLUMN_SKIP		20
+#define MT9M001_ROW_SKIP		12
+
+/* MT9M001 has only one fixed colorspace per pixelcode */
+struct mt9m001_datafmt {
+	u32	code;
+	enum v4l2_colorspace		colorspace;
+};
+
+/* Find a data format by a pixel code in an array */
+static const struct mt9m001_datafmt *mt9m001_find_datafmt(
+	u32 code, const struct mt9m001_datafmt *fmt,
+	int n)
+{
+	int i;
+	for (i = 0; i < n; i++)
+		if (fmt[i].code == code)
+			return fmt + i;
+
+	return NULL;
+}
+
+static const struct mt9m001_datafmt mt9m001_colour_fmts[] = {
+	/*
+	 * Order important: first natively supported,
+	 * second supported with a GPIO extender
+	 */
+	{MEDIA_BUS_FMT_SBGGR10_1X10, V4L2_COLORSPACE_SRGB},
+	{MEDIA_BUS_FMT_SBGGR8_1X8, V4L2_COLORSPACE_SRGB},
+};
+
+static const struct mt9m001_datafmt mt9m001_monochrome_fmts[] = {
+	/* Order important - see above */
+	{MEDIA_BUS_FMT_Y10_1X10, V4L2_COLORSPACE_JPEG},
+	{MEDIA_BUS_FMT_Y8_1X8, V4L2_COLORSPACE_JPEG},
+};
+
+struct mt9m001 {
+	struct v4l2_subdev subdev;
+	struct v4l2_ctrl_handler hdl;
+	struct {
+		/* exposure/auto-exposure cluster */
+		struct v4l2_ctrl *autoexposure;
+		struct v4l2_ctrl *exposure;
+	};
+	struct v4l2_rect rect;	/* Sensor window */
+	struct v4l2_clk *clk;
+	const struct mt9m001_datafmt *fmt;
+	const struct mt9m001_datafmt *fmts;
+	int num_fmts;
+	unsigned int total_h;
+	unsigned short y_skip_top;	/* Lines to skip at the top */
+};
+
+static struct mt9m001 *to_mt9m001(const struct i2c_client *client)
+{
+	return container_of(i2c_get_clientdata(client), struct mt9m001, subdev);
+}
+
+static int reg_read(struct i2c_client *client, const u8 reg)
+{
+	return i2c_smbus_read_word_swapped(client, reg);
+}
+
+static int reg_write(struct i2c_client *client, const u8 reg,
+		     const u16 data)
+{
+	return i2c_smbus_write_word_swapped(client, reg, data);
+}
+
+static int reg_set(struct i2c_client *client, const u8 reg,
+		   const u16 data)
+{
+	int ret;
+
+	ret = reg_read(client, reg);
+	if (ret < 0)
+		return ret;
+	return reg_write(client, reg, ret | data);
+}
+
+static int reg_clear(struct i2c_client *client, const u8 reg,
+		     const u16 data)
+{
+	int ret;
+
+	ret = reg_read(client, reg);
+	if (ret < 0)
+		return ret;
+	return reg_write(client, reg, ret & ~data);
+}
+
+static int mt9m001_init(struct i2c_client *client)
+{
+	int ret;
+
+	dev_dbg(&client->dev, "%s\n", __func__);
+
+	/*
+	 * We don't know, whether platform provides reset, issue a soft reset
+	 * too. This returns all registers to their default values.
+	 */
+	ret = reg_write(client, MT9M001_RESET, 1);
+	if (!ret)
+		ret = reg_write(client, MT9M001_RESET, 0);
+
+	/* Disable chip, synchronous option update */
+	if (!ret)
+		ret = reg_write(client, MT9M001_OUTPUT_CONTROL, 0);
+
+	return ret;
+}
+
+static int mt9m001_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	/* Switch to master "normal" mode or stop sensor readout */
+	if (reg_write(client, MT9M001_OUTPUT_CONTROL, enable ? 2 : 0) < 0)
+		return -EIO;
+	return 0;
+}
+
+static int mt9m001_set_selection(struct v4l2_subdev *sd,
+		struct v4l2_subdev_pad_config *cfg,
+		struct v4l2_subdev_selection *sel)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct mt9m001 *mt9m001 = to_mt9m001(client);
+	struct v4l2_rect rect = sel->r;
+	const u16 hblank = 9, vblank = 25;
+	int ret;
+
+	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE ||
+	    sel->target != V4L2_SEL_TGT_CROP)
+		return -EINVAL;
+
+	if (mt9m001->fmts == mt9m001_colour_fmts)
+		/*
+		 * Bayer format - even number of rows for simplicity,
+		 * but let the user play with the top row.
+		 */
+		rect.height = ALIGN(rect.height, 2);
+
+	/* Datasheet requirement: see register description */
+	rect.width = ALIGN(rect.width, 2);
+	rect.left = ALIGN(rect.left, 2);
+
+	soc_camera_limit_side(&rect.left, &rect.width,
+		     MT9M001_COLUMN_SKIP, MT9M001_MIN_WIDTH, MT9M001_MAX_WIDTH);
+
+	soc_camera_limit_side(&rect.top, &rect.height,
+		     MT9M001_ROW_SKIP, MT9M001_MIN_HEIGHT, MT9M001_MAX_HEIGHT);
+
+	mt9m001->total_h = rect.height + mt9m001->y_skip_top + vblank;
+
+	/* Blanking and start values - default... */
+	ret = reg_write(client, MT9M001_HORIZONTAL_BLANKING, hblank);
+	if (!ret)
+		ret = reg_write(client, MT9M001_VERTICAL_BLANKING, vblank);
+
+	/*
+	 * The caller provides a supported format, as verified per
+	 * call to .set_fmt(FORMAT_TRY).
+	 */
+	if (!ret)
+		ret = reg_write(client, MT9M001_COLUMN_START, rect.left);
+	if (!ret)
+		ret = reg_write(client, MT9M001_ROW_START, rect.top);
+	if (!ret)
+		ret = reg_write(client, MT9M001_WINDOW_WIDTH, rect.width - 1);
+	if (!ret)
+		ret = reg_write(client, MT9M001_WINDOW_HEIGHT,
+				rect.height + mt9m001->y_skip_top - 1);
+	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;
+}
+
+static int mt9m001_get_selection(struct v4l2_subdev *sd,
+		struct v4l2_subdev_pad_config *cfg,
+		struct v4l2_subdev_selection *sel)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct mt9m001 *mt9m001 = to_mt9m001(client);
+
+	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
+		return -EINVAL;
+
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.left = MT9M001_COLUMN_SKIP;
+		sel->r.top = MT9M001_ROW_SKIP;
+		sel->r.width = MT9M001_MAX_WIDTH;
+		sel->r.height = MT9M001_MAX_HEIGHT;
+		return 0;
+	case V4L2_SEL_TGT_CROP:
+		sel->r = mt9m001->rect;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mt9m001_get_fmt(struct v4l2_subdev *sd,
+		struct v4l2_subdev_pad_config *cfg,
+		struct v4l2_subdev_format *format)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct mt9m001 *mt9m001 = to_mt9m001(client);
+	struct v4l2_mbus_framefmt *mf = &format->format;
+
+	if (format->pad)
+		return -EINVAL;
+
+	mf->width	= mt9m001->rect.width;
+	mf->height	= mt9m001->rect.height;
+	mf->code	= mt9m001->fmt->code;
+	mf->colorspace	= mt9m001->fmt->colorspace;
+	mf->field	= V4L2_FIELD_NONE;
+
+	return 0;
+}
+
+static int mt9m001_s_fmt(struct v4l2_subdev *sd,
+			 const struct mt9m001_datafmt *fmt,
+			 struct v4l2_mbus_framefmt *mf)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct mt9m001 *mt9m001 = to_mt9m001(client);
+	struct v4l2_subdev_selection sel = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+		.target = V4L2_SEL_TGT_CROP,
+		.r.left = mt9m001->rect.left,
+		.r.top = mt9m001->rect.top,
+		.r.width = mf->width,
+		.r.height = mf->height,
+	};
+	int ret;
+
+	/* No support for scaling so far, just crop. TODO: use skipping */
+	ret = mt9m001_set_selection(sd, NULL, &sel);
+	if (!ret) {
+		mf->width	= mt9m001->rect.width;
+		mf->height	= mt9m001->rect.height;
+		mt9m001->fmt	= fmt;
+		mf->colorspace	= fmt->colorspace;
+	}
+
+	return ret;
+}
+
+static int mt9m001_set_fmt(struct v4l2_subdev *sd,
+		struct v4l2_subdev_pad_config *cfg,
+		struct v4l2_subdev_format *format)
+{
+	struct v4l2_mbus_framefmt *mf = &format->format;
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct mt9m001 *mt9m001 = to_mt9m001(client);
+	const struct mt9m001_datafmt *fmt;
+
+	if (format->pad)
+		return -EINVAL;
+
+	v4l_bound_align_image(&mf->width, MT9M001_MIN_WIDTH,
+		MT9M001_MAX_WIDTH, 1,
+		&mf->height, MT9M001_MIN_HEIGHT + mt9m001->y_skip_top,
+		MT9M001_MAX_HEIGHT + mt9m001->y_skip_top, 0, 0);
+
+	if (mt9m001->fmts == mt9m001_colour_fmts)
+		mf->height = ALIGN(mf->height - 1, 2);
+
+	fmt = mt9m001_find_datafmt(mf->code, mt9m001->fmts,
+				   mt9m001->num_fmts);
+	if (!fmt) {
+		fmt = mt9m001->fmt;
+		mf->code = fmt->code;
+	}
+
+	mf->colorspace	= fmt->colorspace;
+
+	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
+		return mt9m001_s_fmt(sd, fmt, mf);
+	cfg->try_fmt = *mf;
+	return 0;
+}
+
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+static int mt9m001_g_register(struct v4l2_subdev *sd,
+			      struct v4l2_dbg_register *reg)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	if (reg->reg > 0xff)
+		return -EINVAL;
+
+	reg->size = 2;
+	reg->val = reg_read(client, reg->reg);
+
+	if (reg->val > 0xffff)
+		return -EIO;
+
+	return 0;
+}
+
+static int mt9m001_s_register(struct v4l2_subdev *sd,
+			      const struct v4l2_dbg_register *reg)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+
+	if (reg->reg > 0xff)
+		return -EINVAL;
+
+	if (reg_write(client, reg->reg, reg->val) < 0)
+		return -EIO;
+
+	return 0;
+}
+#endif
+
+static int mt9m001_s_power(struct v4l2_subdev *sd, int on)
+{
+	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);
+
+	return soc_camera_set_power(&client->dev, ssdd, mt9m001->clk, on);
+}
+
+static int mt9m001_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct mt9m001 *mt9m001 = container_of(ctrl->handler,
+					       struct mt9m001, hdl);
+	s32 min, max;
+
+	switch (ctrl->id) {
+	case V4L2_CID_EXPOSURE_AUTO:
+		min = mt9m001->exposure->minimum;
+		max = mt9m001->exposure->maximum;
+		mt9m001->exposure->val =
+			(524 + (mt9m001->total_h - 1) * (max - min)) / 1048 + min;
+		break;
+	}
+	return 0;
+}
+
+static int mt9m001_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct mt9m001 *mt9m001 = container_of(ctrl->handler,
+					       struct mt9m001, hdl);
+	struct v4l2_subdev *sd = &mt9m001->subdev;
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct v4l2_ctrl *exp = mt9m001->exposure;
+	int data;
+
+	switch (ctrl->id) {
+	case V4L2_CID_VFLIP:
+		if (ctrl->val)
+			data = reg_set(client, MT9M001_READ_OPTIONS2, 0x8000);
+		else
+			data = reg_clear(client, MT9M001_READ_OPTIONS2, 0x8000);
+		if (data < 0)
+			return -EIO;
+		return 0;
+
+	case V4L2_CID_GAIN:
+		/* See Datasheet Table 7, Gain settings. */
+		if (ctrl->val <= ctrl->default_value) {
+			/* Pack it into 0..1 step 0.125, register values 0..8 */
+			unsigned long range = ctrl->default_value - ctrl->minimum;
+			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;
+		} else {
+			/* Pack it into 1.125..15 variable step, register values 9..67 */
+			/* We assume qctrl->maximum - qctrl->default_value - 1 > 0 */
+			unsigned long range = ctrl->maximum - ctrl->default_value - 1;
+			unsigned long gain = ((ctrl->val - (s32)ctrl->default_value - 1) *
+					       111 + range / 2) / range + 9;
+
+			if (gain <= 32)
+				data = gain;
+			else if (gain <= 64)
+				data = ((gain - 32) * 16 + 16) / 32 + 80;
+			else
+				data = ((gain - 64) * 7 + 28) / 56 + 96;
+
+			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;
+		}
+		return 0;
+
+	case V4L2_CID_EXPOSURE_AUTO:
+		if (ctrl->val == V4L2_EXPOSURE_MANUAL) {
+			unsigned long range = exp->maximum - exp->minimum;
+			unsigned long shutter = ((exp->val - (s32)exp->minimum) * 1048 +
+						 range / 2) / range + 1;
+
+			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;
+		} 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;
+		}
+		return 0;
+	}
+	return -EINVAL;
+}
+
+/*
+ * Interface active, can use i2c. If it fails, it can indeed mean, that
+ * this wasn't our capture interface, so, we wait for the right one
+ */
+static int mt9m001_video_probe(struct soc_camera_subdev_desc *ssdd,
+			       struct i2c_client *client)
+{
+	struct mt9m001 *mt9m001 = to_mt9m001(client);
+	s32 data;
+	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);
+
+	/* Read out the chip version register */
+	data = reg_read(client, MT9M001_CHIP_VERSION);
+
+	/* must be 0x8411 or 0x8421 for colour sensor and 8431 for bw */
+	switch (data) {
+	case 0x8411:
+	case 0x8421:
+		mt9m001->fmts = mt9m001_colour_fmts;
+		break;
+	case 0x8431:
+		mt9m001->fmts = mt9m001_monochrome_fmts;
+		break;
+	default:
+		dev_err(&client->dev,
+			"No MT9M001 chip detected, register read %x\n", data);
+		ret = -ENODEV;
+		goto done;
+	}
+
+	mt9m001->num_fmts = 0;
+
+	/*
+	 * This is a 10bit sensor, so by default we only allow 10bit.
+	 * The platform may support different bus widths due to
+	 * different routing of the data lines.
+	 */
+	if (ssdd->query_bus_param)
+		flags = ssdd->query_bus_param(ssdd);
+	else
+		flags = SOCAM_DATAWIDTH_10;
+
+	if (flags & SOCAM_DATAWIDTH_10)
+		mt9m001->num_fmts++;
+	else
+		mt9m001->fmts++;
+
+	if (flags & SOCAM_DATAWIDTH_8)
+		mt9m001->num_fmts++;
+
+	mt9m001->fmt = &mt9m001->fmts[0];
+
+	dev_info(&client->dev, "Detected a MT9M001 chip ID %x (%s)\n", data,
+		 data == 0x8431 ? "C12STM" : "C12ST");
+
+	ret = mt9m001_init(client);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to initialise the camera\n");
+		goto done;
+	}
+
+	/* mt9m001_init() has reset the chip, returning registers to defaults */
+	ret = v4l2_ctrl_handler_setup(&mt9m001->hdl);
+
+done:
+	mt9m001_s_power(&mt9m001->subdev, 0);
+	return ret;
+}
+
+static void mt9m001_video_remove(struct soc_camera_subdev_desc *ssdd)
+{
+	if (ssdd->free_bus)
+		ssdd->free_bus(ssdd);
+}
+
+static int mt9m001_g_skip_top_lines(struct v4l2_subdev *sd, u32 *lines)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct mt9m001 *mt9m001 = to_mt9m001(client);
+
+	*lines = mt9m001->y_skip_top;
+
+	return 0;
+}
+
+static const struct v4l2_ctrl_ops mt9m001_ctrl_ops = {
+	.g_volatile_ctrl = mt9m001_g_volatile_ctrl,
+	.s_ctrl = mt9m001_s_ctrl,
+};
+
+static const struct v4l2_subdev_core_ops mt9m001_subdev_core_ops = {
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+	.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,
+		struct v4l2_subdev_pad_config *cfg,
+		struct v4l2_subdev_mbus_code_enum *code)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct mt9m001 *mt9m001 = to_mt9m001(client);
+
+	if (code->pad || code->index >= mt9m001->num_fmts)
+		return -EINVAL;
+
+	code->code = mt9m001->fmts[code->index].code;
+	return 0;
+}
+
+static int mt9m001_g_mbus_config(struct v4l2_subdev *sd,
+				struct v4l2_mbus_config *cfg)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+
+	/* MT9M001 has all capture_format parameters fixed */
+	cfg->flags = V4L2_MBUS_PCLK_SAMPLE_FALLING |
+		V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_HIGH |
+		V4L2_MBUS_DATA_ACTIVE_HIGH | V4L2_MBUS_MASTER;
+	cfg->type = V4L2_MBUS_PARALLEL;
+	cfg->flags = soc_camera_apply_board_flags(ssdd, cfg);
+
+	return 0;
+}
+
+static int mt9m001_s_mbus_config(struct v4l2_subdev *sd,
+				const struct v4l2_mbus_config *cfg)
+{
+	const 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);
+	unsigned int bps = soc_mbus_get_fmtdesc(mt9m001->fmt->code)->bits_per_sample;
+
+	if (ssdd->set_bus_param)
+		return ssdd->set_bus_param(ssdd, 1 << (bps - 1));
+
+	/*
+	 * Without board specific bus width settings we only support the
+	 * sensors native bus width
+	 */
+	return bps == 10 ? 0 : -EINVAL;
+}
+
+static const struct v4l2_subdev_video_ops mt9m001_subdev_video_ops = {
+	.s_stream	= mt9m001_s_stream,
+	.g_mbus_config	= mt9m001_g_mbus_config,
+	.s_mbus_config	= mt9m001_s_mbus_config,
+};
+
+static const struct v4l2_subdev_sensor_ops mt9m001_subdev_sensor_ops = {
+	.g_skip_top_lines	= mt9m001_g_skip_top_lines,
+};
+
+static const struct v4l2_subdev_pad_ops mt9m001_subdev_pad_ops = {
+	.enum_mbus_code = mt9m001_enum_mbus_code,
+	.get_selection	= mt9m001_get_selection,
+	.set_selection	= mt9m001_set_selection,
+	.get_fmt	= mt9m001_get_fmt,
+	.set_fmt	= mt9m001_set_fmt,
+};
+
+static const struct v4l2_subdev_ops mt9m001_subdev_ops = {
+	.core	= &mt9m001_subdev_core_ops,
+	.video	= &mt9m001_subdev_video_ops,
+	.sensor	= &mt9m001_subdev_sensor_ops,
+	.pad	= &mt9m001_subdev_pad_ops,
+};
+
+static int mt9m001_probe(struct i2c_client *client,
+			 const struct i2c_device_id *did)
+{
+	struct mt9m001 *mt9m001;
+	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
+	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
+	int ret;
+
+	if (!ssdd) {
+		dev_err(&client->dev, "MT9M001 driver needs platform data\n");
+		return -EINVAL;
+	}
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
+		dev_warn(&adapter->dev,
+			 "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
+		return -EIO;
+	}
+
+	mt9m001 = devm_kzalloc(&client->dev, sizeof(struct mt9m001), GFP_KERNEL);
+	if (!mt9m001)
+		return -ENOMEM;
+
+	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,
+			V4L2_CID_VFLIP, 0, 1, 1, 0);
+	v4l2_ctrl_new_std(&mt9m001->hdl, &mt9m001_ctrl_ops,
+			V4L2_CID_GAIN, 0, 127, 1, 64);
+	mt9m001->exposure = v4l2_ctrl_new_std(&mt9m001->hdl, &mt9m001_ctrl_ops,
+			V4L2_CID_EXPOSURE, 1, 255, 1, 255);
+	/*
+	 * Simulated autoexposure. If enabled, we calculate shutter width
+	 * ourselves in the driver based on vertical blanking and frame width
+	 */
+	mt9m001->autoexposure = v4l2_ctrl_new_std_menu(&mt9m001->hdl,
+			&mt9m001_ctrl_ops, V4L2_CID_EXPOSURE_AUTO, 1, 0,
+			V4L2_EXPOSURE_AUTO);
+	mt9m001->subdev.ctrl_handler = &mt9m001->hdl;
+	if (mt9m001->hdl.error)
+		return mt9m001->hdl.error;
+
+	v4l2_ctrl_auto_cluster(2, &mt9m001->autoexposure,
+					V4L2_EXPOSURE_MANUAL, true);
+
+	/* Second stage probe - when a capture adapter is there */
+	mt9m001->y_skip_top	= 0;
+	mt9m001->rect.left	= MT9M001_COLUMN_SKIP;
+	mt9m001->rect.top	= MT9M001_ROW_SKIP;
+	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_video_probe(ssdd, client);
+	if (ret) {
+		v4l2_clk_put(mt9m001->clk);
+eclkget:
+		v4l2_ctrl_handler_free(&mt9m001->hdl);
+	}
+
+	return ret;
+}
+
+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);
+	v4l2_ctrl_handler_free(&mt9m001->hdl);
+	mt9m001_video_remove(ssdd);
+
+	return 0;
+}
+
+static const struct i2c_device_id mt9m001_id[] = {
+	{ "mt9m001", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, mt9m001_id);
+
+static struct i2c_driver mt9m001_i2c_driver = {
+	.driver = {
+		.name = "mt9m001",
+	},
+	.probe		= mt9m001_probe,
+	.remove		= mt9m001_remove,
+	.id_table	= mt9m001_id,
+};
+
+module_i2c_driver(mt9m001_i2c_driver);
+
+MODULE_DESCRIPTION("Micron MT9M001 Camera driver");
+MODULE_AUTHOR("Guennadi Liakhovetski <kernel@pengutronix.de>");
+MODULE_LICENSE("GPL");
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 02/12] media: i2c: mt9m001: dt: add binding for mt9m001
  2018-12-22 17:12 [PATCH 00/12] media: mt9m001: switch soc_mt9m001 to a standard subdev sensor driver Akinobu Mita
  2018-12-22 17:12 ` [PATCH 01/12] media: i2c: mt9m001: copy mt9m001 soc_camera " Akinobu Mita
@ 2018-12-22 17:12 ` Akinobu Mita
  2019-01-03 21:12   ` Rob Herring
  2018-12-22 17:12 ` [PATCH 03/12] media: mt9m001: convert to SPDX license identifer Akinobu Mita
                   ` (10 subsequent siblings)
  12 siblings, 1 reply; 31+ messages in thread
From: Akinobu Mita @ 2018-12-22 17:12 UTC (permalink / raw)
  To: linux-media, devicetree
  Cc: Akinobu Mita, Rob Herring, Guennadi Liakhovetski, Sakari Ailus,
	Mauro Carvalho Chehab

Add device tree binding documentation for the MT9M001 CMOS image sensor.

Cc: Rob Herring <robh@kernel.org>
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>
---
 .../devicetree/bindings/media/i2c/mt9m001.txt      | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/mt9m001.txt

diff --git a/Documentation/devicetree/bindings/media/i2c/mt9m001.txt b/Documentation/devicetree/bindings/media/i2c/mt9m001.txt
new file mode 100644
index 0000000..794b787
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/i2c/mt9m001.txt
@@ -0,0 +1,37 @@
+MT9M001: 1/2-Inch Megapixel Digital Image Sensor
+
+The MT9M001 is an SXGA-format with a 1/2-inch CMOS active-pixel digital
+image sensor. It is programmable through a simple two-wire serial
+interface.
+
+Required Properties:
+
+- compatible: shall be "onnn,mt9m001".
+- clocks: reference to the master clock into sensor
+
+Optional Properties:
+
+- reset-gpios: GPIO handle which is connected to the reset pin of the chip.
+  Active low.
+- standby-gpios: GPIO handle which is connected to the standby pin of the chip.
+  Active high.
+
+For further reading on port node refer to
+Documentation/devicetree/bindings/media/video-interfaces.txt.
+
+Example:
+
+	&i2c1 {
+		mt9m001@5d {
+			compatible = "onnn,mt9m001";
+			reg = <0x5d>;
+			reset-gpios = <&gpio0 0 GPIO_ACTIVE_LOW>;
+			standby-gpios = <&gpio0 1 GPIO_ACTIVE_HIGH>;
+			clocks = <&camera_clk>;
+			port {
+				mt9m001_out: endpoint {
+					remote-endpoint = <&vcap_in>;
+				};
+			};
+		};
+	};
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 03/12] media: mt9m001: convert to SPDX license identifer
  2018-12-22 17:12 [PATCH 00/12] media: mt9m001: switch soc_mt9m001 to a standard subdev sensor driver Akinobu Mita
  2018-12-22 17:12 ` [PATCH 01/12] media: i2c: mt9m001: copy mt9m001 soc_camera " Akinobu Mita
  2018-12-22 17:12 ` [PATCH 02/12] media: i2c: mt9m001: dt: add binding for mt9m001 Akinobu Mita
@ 2018-12-22 17:12 ` Akinobu Mita
  2019-01-07 11:37   ` Sakari Ailus
  2018-12-22 17:12 ` [PATCH 04/12] media: mt9m001: add of_match_table Akinobu Mita
                   ` (9 subsequent siblings)
  12 siblings, 1 reply; 31+ messages in thread
From: Akinobu Mita @ 2018-12-22 17:12 UTC (permalink / raw)
  To: linux-media, devicetree
  Cc: Akinobu Mita, Guennadi Liakhovetski, Sakari Ailus, Mauro Carvalho Chehab

Replace GPL license statements with SPDX license identifiers (GPL-2.0).

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 | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
index a1a85ff..65ff59d 100644
--- a/drivers/media/i2c/mt9m001.c
+++ b/drivers/media/i2c/mt9m001.c
@@ -1,11 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * Driver for MT9M001 CMOS Image Sensor from Micron
  *
  * Copyright (C) 2008, Guennadi Liakhovetski <kernel@pengutronix.de>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
  */
 
 #include <linux/videodev2.h>
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 04/12] media: mt9m001: add of_match_table
  2018-12-22 17:12 [PATCH 00/12] media: mt9m001: switch soc_mt9m001 to a standard subdev sensor driver Akinobu Mita
                   ` (2 preceding siblings ...)
  2018-12-22 17:12 ` [PATCH 03/12] media: mt9m001: convert to SPDX license identifer Akinobu Mita
@ 2018-12-22 17:12 ` Akinobu Mita
  2018-12-22 17:12 ` [PATCH 05/12] media: mt9m001: introduce multi_reg_write() Akinobu Mita
                   ` (8 subsequent siblings)
  12 siblings, 0 replies; 31+ messages in thread
From: Akinobu Mita @ 2018-12-22 17:12 UTC (permalink / raw)
  To: linux-media, devicetree
  Cc: Akinobu Mita, Guennadi Liakhovetski, Sakari Ailus, Mauro Carvalho Chehab

Add of_match_table for the MT9M001 CMOS image sensor.

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 | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
index 65ff59d..2d800ca 100644
--- a/drivers/media/i2c/mt9m001.c
+++ b/drivers/media/i2c/mt9m001.c
@@ -738,9 +738,16 @@ static const struct i2c_device_id mt9m001_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, mt9m001_id);
 
+static const struct of_device_id mt9m001_of_match[] = {
+	{ .compatible = "onnn,mt9m001", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, mt9m001_of_match);
+
 static struct i2c_driver mt9m001_i2c_driver = {
 	.driver = {
 		.name = "mt9m001",
+		.of_match_table = mt9m001_of_match,
 	},
 	.probe		= mt9m001_probe,
 	.remove		= mt9m001_remove,
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 05/12] media: mt9m001: introduce multi_reg_write()
  2018-12-22 17:12 [PATCH 00/12] media: mt9m001: switch soc_mt9m001 to a standard subdev sensor driver Akinobu Mita
                   ` (3 preceding siblings ...)
  2018-12-22 17:12 ` [PATCH 04/12] media: mt9m001: add of_match_table Akinobu Mita
@ 2018-12-22 17:12 ` Akinobu Mita
  2018-12-22 17:12 ` [PATCH 06/12] media: mt9m001: switch s_power callback to runtime PM Akinobu Mita
                   ` (7 subsequent siblings)
  12 siblings, 0 replies; 31+ messages in thread
From: Akinobu Mita @ 2018-12-22 17:12 UTC (permalink / raw)
  To: linux-media, devicetree
  Cc: Akinobu Mita, Guennadi Liakhovetski, Sakari Ailus, Mauro Carvalho Chehab

Introduce multi_reg_write() to write multiple registers to the device and
use it where possible.

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 | 88 +++++++++++++++++++++++++++++----------------
 1 file changed, 57 insertions(+), 31 deletions(-)

diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
index 2d800ca..c0180fdc 100644
--- a/drivers/media/i2c/mt9m001.c
+++ b/drivers/media/i2c/mt9m001.c
@@ -47,6 +47,8 @@
 #define MT9M001_MIN_HEIGHT		32
 #define MT9M001_COLUMN_SKIP		20
 #define MT9M001_ROW_SKIP		12
+#define MT9M001_DEFAULT_HBLANK		9
+#define MT9M001_DEFAULT_VBLANK		25
 
 /* MT9M001 has only one fixed colorspace per pixelcode */
 struct mt9m001_datafmt {
@@ -137,25 +139,65 @@ static int reg_clear(struct i2c_client *client, const u8 reg,
 	return reg_write(client, reg, ret & ~data);
 }
 
+struct mt9m001_reg {
+	u8 reg;
+	u16 data;
+};
+
+static int multi_reg_write(struct i2c_client *client,
+			   const struct mt9m001_reg *regs, int num)
+{
+	int i;
+
+	for (i = 0; i < num; i++) {
+		int ret = reg_write(client, regs[i].reg, regs[i].data);
+
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int mt9m001_init(struct i2c_client *client)
 {
-	int ret;
+	const struct mt9m001_reg init_regs[] = {
+		/*
+		 * Issue a soft reset. This returns all registers to their
+		 * default values.
+		 */
+		{ MT9M001_RESET, 1 },
+		{ MT9M001_RESET, 0 },
+		/* Disable chip, synchronous option update */
+		{ MT9M001_OUTPUT_CONTROL, 0 }
+	};
 
 	dev_dbg(&client->dev, "%s\n", __func__);
 
-	/*
-	 * We don't know, whether platform provides reset, issue a soft reset
-	 * too. This returns all registers to their default values.
-	 */
-	ret = reg_write(client, MT9M001_RESET, 1);
-	if (!ret)
-		ret = reg_write(client, MT9M001_RESET, 0);
+	return multi_reg_write(client, init_regs, ARRAY_SIZE(init_regs));
+}
 
-	/* Disable chip, synchronous option update */
-	if (!ret)
-		ret = reg_write(client, MT9M001_OUTPUT_CONTROL, 0);
+static int mt9m001_apply_selection(struct v4l2_subdev *sd,
+				    struct v4l2_rect *rect)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct mt9m001 *mt9m001 = to_mt9m001(client);
+	const struct mt9m001_reg regs[] = {
+		/* Blanking and start values - default... */
+		{ MT9M001_HORIZONTAL_BLANKING, MT9M001_DEFAULT_HBLANK },
+		{ MT9M001_VERTICAL_BLANKING, MT9M001_DEFAULT_VBLANK },
+		/*
+		 * 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_WINDOW_HEIGHT,
+			rect->height + mt9m001->y_skip_top - 1 },
+	};
 
-	return ret;
+	return multi_reg_write(client, regs, ARRAY_SIZE(regs));
 }
 
 static int mt9m001_s_stream(struct v4l2_subdev *sd, int enable)
@@ -175,7 +217,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;
-	const u16 hblank = 9, vblank = 25;
 	int ret;
 
 	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE ||
@@ -199,26 +240,11 @@ static int mt9m001_set_selection(struct v4l2_subdev *sd,
 	soc_camera_limit_side(&rect.top, &rect.height,
 		     MT9M001_ROW_SKIP, MT9M001_MIN_HEIGHT, MT9M001_MAX_HEIGHT);
 
-	mt9m001->total_h = rect.height + mt9m001->y_skip_top + vblank;
+	mt9m001->total_h = rect.height + mt9m001->y_skip_top +
+			   MT9M001_DEFAULT_VBLANK;
 
-	/* Blanking and start values - default... */
-	ret = reg_write(client, MT9M001_HORIZONTAL_BLANKING, hblank);
-	if (!ret)
-		ret = reg_write(client, MT9M001_VERTICAL_BLANKING, vblank);
 
-	/*
-	 * The caller provides a supported format, as verified per
-	 * call to .set_fmt(FORMAT_TRY).
-	 */
-	if (!ret)
-		ret = reg_write(client, MT9M001_COLUMN_START, rect.left);
-	if (!ret)
-		ret = reg_write(client, MT9M001_ROW_START, rect.top);
-	if (!ret)
-		ret = reg_write(client, MT9M001_WINDOW_WIDTH, rect.width - 1);
-	if (!ret)
-		ret = reg_write(client, MT9M001_WINDOW_HEIGHT,
-				rect.height + mt9m001->y_skip_top - 1);
+	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);
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 06/12] media: mt9m001: switch s_power callback to runtime PM
  2018-12-22 17:12 [PATCH 00/12] media: mt9m001: switch soc_mt9m001 to a standard subdev sensor driver Akinobu Mita
                   ` (4 preceding siblings ...)
  2018-12-22 17:12 ` [PATCH 05/12] media: mt9m001: introduce multi_reg_write() Akinobu Mita
@ 2018-12-22 17:12 ` Akinobu Mita
       [not found]   ` <20190107100034.po3jsnc3rdj37l4x@kekkonen.localdomain>
  2018-12-22 17:12 ` [PATCH 07/12] media: mt9m001: remove remaining soc_camera specific code Akinobu Mita
                   ` (6 subsequent siblings)
  12 siblings, 1 reply; 31+ messages in thread
From: Akinobu Mita @ 2018-12-22 17:12 UTC (permalink / raw)
  To: linux-media, devicetree
  Cc: Akinobu Mita, Guennadi Liakhovetski, Sakari Ailus, Mauro Carvalho Chehab

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;
+		}
+
+		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);
 
 	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);
+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);
+}
+
+static int __maybe_unused mt9m001_runtime_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct mt9m001 *mt9m001 = to_mt9m001(client);
+
+	mt9m001_power_off(mt9m001);
+
+	return 0;
+}
+
+static const struct dev_pm_ops mt9m001_pm_ops = {
+	SET_RUNTIME_PM_OPS(mt9m001_runtime_suspend,
+			   mt9m001_runtime_resume, NULL)
+};
+
 static const struct of_device_id mt9m001_of_match[] = {
 	{ .compatible = "onnn,mt9m001", },
 	{ /* sentinel */ },
@@ -773,6 +886,7 @@ MODULE_DEVICE_TABLE(of, mt9m001_of_match);
 static struct i2c_driver mt9m001_i2c_driver = {
 	.driver = {
 		.name = "mt9m001",
+		.pm = &mt9m001_pm_ops,
 		.of_match_table = mt9m001_of_match,
 	},
 	.probe		= mt9m001_probe,
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 07/12] media: mt9m001: remove remaining soc_camera specific code
  2018-12-22 17:12 [PATCH 00/12] media: mt9m001: switch soc_mt9m001 to a standard subdev sensor driver Akinobu Mita
                   ` (5 preceding siblings ...)
  2018-12-22 17:12 ` [PATCH 06/12] media: mt9m001: switch s_power callback to runtime PM Akinobu Mita
@ 2018-12-22 17:12 ` Akinobu Mita
  2019-01-07 11:29   ` Sakari Ailus
  2018-12-22 17:12 ` [PATCH 08/12] media: mt9m001: add media controller support Akinobu Mita
                   ` (5 subsequent siblings)
  12 siblings, 1 reply; 31+ messages in thread
From: Akinobu Mita @ 2018-12-22 17:12 UTC (permalink / raw)
  To: linux-media, devicetree
  Cc: Akinobu Mita, Guennadi Liakhovetski, Sakari Ailus, Mauro Carvalho Chehab

Remove remaining soc_camera specific code and drop soc_camera dependency
from this driver.

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/Kconfig   |  2 +-
 drivers/media/i2c/mt9m001.c | 84 ++++++++-------------------------------------
 2 files changed, 15 insertions(+), 71 deletions(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 0efc038..4bdf043 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -845,7 +845,7 @@ config VIDEO_VS6624
 
 config VIDEO_MT9M001
 	tristate "mt9m001 support"
-	depends on SOC_CAMERA && I2C
+	depends on I2C && VIDEO_V4L2
 	help
 	  This driver supports MT9M001 cameras from Micron, monochrome
 	  and colour models.
diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
index f20188a..eb5c4ed 100644
--- a/drivers/media/i2c/mt9m001.c
+++ b/drivers/media/i2c/mt9m001.c
@@ -15,15 +15,12 @@
 #include <linux/log2.h>
 #include <linux/module.h>
 
-#include <media/soc_camera.h>
-#include <media/drv-intf/soc_mediabus.h>
 #include <media/v4l2-subdev.h>
 #include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
 
 /*
  * mt9m001 i2c address 0x5d
- * The platform has to define struct i2c_board_info objects and link to them
- * from struct soc_camera_host_desc
  */
 
 /* mt9m001 selected register addresses */
@@ -278,11 +275,15 @@ static int mt9m001_set_selection(struct v4l2_subdev *sd,
 	rect.width = ALIGN(rect.width, 2);
 	rect.left = ALIGN(rect.left, 2);
 
-	soc_camera_limit_side(&rect.left, &rect.width,
-		     MT9M001_COLUMN_SKIP, MT9M001_MIN_WIDTH, MT9M001_MAX_WIDTH);
+	rect.width = clamp_t(u32, rect.width, MT9M001_MIN_WIDTH,
+			MT9M001_MAX_WIDTH);
+	rect.left = clamp_t(u32, rect.left, MT9M001_COLUMN_SKIP,
+			MT9M001_COLUMN_SKIP + MT9M001_MAX_WIDTH - rect.width);
 
-	soc_camera_limit_side(&rect.top, &rect.height,
-		     MT9M001_ROW_SKIP, MT9M001_MIN_HEIGHT, MT9M001_MAX_HEIGHT);
+	rect.height = clamp_t(u32, rect.height, MT9M001_MIN_HEIGHT,
+			MT9M001_MAX_HEIGHT);
+	rect.top = clamp_t(u32, rect.top, MT9M001_ROW_SKIP,
+			MT9M001_ROW_SKIP + MT9M001_MAX_HEIGHT - rect.width);
 
 	mt9m001->total_h = rect.height + mt9m001->y_skip_top +
 			   MT9M001_DEFAULT_VBLANK;
@@ -561,12 +562,10 @@ static int mt9m001_s_ctrl(struct v4l2_ctrl *ctrl)
  * Interface active, can use i2c. If it fails, it can indeed mean, that
  * this wasn't our capture interface, so, we wait for the right one
  */
-static int mt9m001_video_probe(struct soc_camera_subdev_desc *ssdd,
-			       struct i2c_client *client)
+static int mt9m001_video_probe(struct i2c_client *client)
 {
 	struct mt9m001 *mt9m001 = to_mt9m001(client);
 	s32 data;
-	unsigned long flags;
 	int ret;
 
 	/* Enable the chip */
@@ -581,9 +580,11 @@ static int mt9m001_video_probe(struct soc_camera_subdev_desc *ssdd,
 	case 0x8411:
 	case 0x8421:
 		mt9m001->fmts = mt9m001_colour_fmts;
+		mt9m001->num_fmts = ARRAY_SIZE(mt9m001_colour_fmts);
 		break;
 	case 0x8431:
 		mt9m001->fmts = mt9m001_monochrome_fmts;
+		mt9m001->num_fmts = ARRAY_SIZE(mt9m001_monochrome_fmts);
 		break;
 	default:
 		dev_err(&client->dev,
@@ -592,26 +593,6 @@ static int mt9m001_video_probe(struct soc_camera_subdev_desc *ssdd,
 		goto done;
 	}
 
-	mt9m001->num_fmts = 0;
-
-	/*
-	 * This is a 10bit sensor, so by default we only allow 10bit.
-	 * The platform may support different bus widths due to
-	 * different routing of the data lines.
-	 */
-	if (ssdd->query_bus_param)
-		flags = ssdd->query_bus_param(ssdd);
-	else
-		flags = SOCAM_DATAWIDTH_10;
-
-	if (flags & SOCAM_DATAWIDTH_10)
-		mt9m001->num_fmts++;
-	else
-		mt9m001->fmts++;
-
-	if (flags & SOCAM_DATAWIDTH_8)
-		mt9m001->num_fmts++;
-
 	mt9m001->fmt = &mt9m001->fmts[0];
 
 	dev_info(&client->dev, "Detected a MT9M001 chip ID %x (%s)\n", data,
@@ -630,12 +611,6 @@ static int mt9m001_video_probe(struct soc_camera_subdev_desc *ssdd,
 	return ret;
 }
 
-static void mt9m001_video_remove(struct soc_camera_subdev_desc *ssdd)
-{
-	if (ssdd->free_bus)
-		ssdd->free_bus(ssdd);
-}
-
 static int mt9m001_g_skip_top_lines(struct v4l2_subdev *sd, u32 *lines)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
@@ -675,41 +650,18 @@ static int mt9m001_enum_mbus_code(struct v4l2_subdev *sd,
 static int mt9m001_g_mbus_config(struct v4l2_subdev *sd,
 				struct v4l2_mbus_config *cfg)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
-	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
-
 	/* MT9M001 has all capture_format parameters fixed */
 	cfg->flags = V4L2_MBUS_PCLK_SAMPLE_FALLING |
 		V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_HIGH |
 		V4L2_MBUS_DATA_ACTIVE_HIGH | V4L2_MBUS_MASTER;
 	cfg->type = V4L2_MBUS_PARALLEL;
-	cfg->flags = soc_camera_apply_board_flags(ssdd, cfg);
 
 	return 0;
 }
 
-static int mt9m001_s_mbus_config(struct v4l2_subdev *sd,
-				const struct v4l2_mbus_config *cfg)
-{
-	const 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);
-	unsigned int bps = soc_mbus_get_fmtdesc(mt9m001->fmt->code)->bits_per_sample;
-
-	if (ssdd->set_bus_param)
-		return ssdd->set_bus_param(ssdd, 1 << (bps - 1));
-
-	/*
-	 * Without board specific bus width settings we only support the
-	 * sensors native bus width
-	 */
-	return bps == 10 ? 0 : -EINVAL;
-}
-
 static const struct v4l2_subdev_video_ops mt9m001_subdev_video_ops = {
 	.s_stream	= mt9m001_s_stream,
 	.g_mbus_config	= mt9m001_g_mbus_config,
-	.s_mbus_config	= mt9m001_s_mbus_config,
 };
 
 static const struct v4l2_subdev_sensor_ops mt9m001_subdev_sensor_ops = {
@@ -736,21 +688,15 @@ static int mt9m001_probe(struct i2c_client *client,
 {
 	struct mt9m001 *mt9m001;
 	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
-	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
 	int ret;
 
-	if (!ssdd) {
-		dev_err(&client->dev, "MT9M001 driver needs platform data\n");
-		return -EINVAL;
-	}
-
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
 		dev_warn(&adapter->dev,
 			 "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
 		return -EIO;
 	}
 
-	mt9m001 = devm_kzalloc(&client->dev, sizeof(struct mt9m001), GFP_KERNEL);
+	mt9m001 = devm_kzalloc(&client->dev, sizeof(*mt9m001), GFP_KERNEL);
 	if (!mt9m001)
 		return -ENOMEM;
 
@@ -808,7 +754,7 @@ static int mt9m001_probe(struct i2c_client *client,
 	pm_runtime_set_active(&client->dev);
 	pm_runtime_enable(&client->dev);
 
-	ret = mt9m001_video_probe(ssdd, client);
+	ret = mt9m001_video_probe(client);
 	if (ret)
 		goto error_power_off;
 
@@ -831,7 +777,6 @@ static int mt9m001_probe(struct i2c_client *client,
 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_device_unregister_subdev(&mt9m001->subdev);
 	pm_runtime_get_sync(&client->dev);
@@ -842,7 +787,6 @@ static int mt9m001_remove(struct i2c_client *client)
 	mt9m001_power_off(mt9m001);
 
 	v4l2_ctrl_handler_free(&mt9m001->hdl);
-	mt9m001_video_remove(ssdd);
 	mutex_destroy(&mt9m001->mutex);
 
 	return 0;
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 08/12] media: mt9m001: add media controller support
  2018-12-22 17:12 [PATCH 00/12] media: mt9m001: switch soc_mt9m001 to a standard subdev sensor driver Akinobu Mita
                   ` (6 preceding siblings ...)
  2018-12-22 17:12 ` [PATCH 07/12] media: mt9m001: remove remaining soc_camera specific code Akinobu Mita
@ 2018-12-22 17:12 ` Akinobu Mita
  2018-12-22 17:12 ` [PATCH 09/12] media: mt9m001: register to V4L2 asynchronous subdevice framework Akinobu Mita
                   ` (4 subsequent siblings)
  12 siblings, 0 replies; 31+ messages in thread
From: Akinobu Mita @ 2018-12-22 17:12 UTC (permalink / raw)
  To: linux-media, devicetree
  Cc: Akinobu Mita, Guennadi Liakhovetski, Sakari Ailus, Mauro Carvalho Chehab

Create a source pad and set the media controller type to the sensor.

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/Kconfig   | 2 ++
 drivers/media/i2c/mt9m001.c | 9 +++++++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 4bdf043..5e30ad3 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -846,6 +846,8 @@ config VIDEO_VS6624
 config VIDEO_MT9M001
 	tristate "mt9m001 support"
 	depends on I2C && VIDEO_V4L2
+	depends on MEDIA_CAMERA_SUPPORT
+	depends on MEDIA_CONTROLLER
 	help
 	  This driver supports MT9M001 cameras from Micron, monochrome
 	  and colour models.
diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
index eb5c4ed..e31fb7d 100644
--- a/drivers/media/i2c/mt9m001.c
+++ b/drivers/media/i2c/mt9m001.c
@@ -103,6 +103,7 @@ struct mt9m001 {
 	int num_fmts;
 	unsigned int total_h;
 	unsigned short y_skip_top;	/* Lines to skip at the top */
+	struct media_pad pad;
 };
 
 static struct mt9m001 *to_mt9m001(const struct i2c_client *client)
@@ -758,6 +759,12 @@ static int mt9m001_probe(struct i2c_client *client,
 	if (ret)
 		goto error_power_off;
 
+	mt9m001->pad.flags = MEDIA_PAD_FL_SOURCE;
+	mt9m001->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+	ret = media_entity_pads_init(&mt9m001->subdev.entity, 1, &mt9m001->pad);
+	if (ret)
+		goto error_power_off;
+
 	pm_runtime_put_sync(&client->dev);
 
 	return 0;
@@ -781,6 +788,8 @@ static int mt9m001_remove(struct i2c_client *client)
 	v4l2_device_unregister_subdev(&mt9m001->subdev);
 	pm_runtime_get_sync(&client->dev);
 
+	media_entity_cleanup(&mt9m001->subdev.entity);
+
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
 	pm_runtime_put_noidle(&client->dev);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 09/12] media: mt9m001: register to V4L2 asynchronous subdevice framework
  2018-12-22 17:12 [PATCH 00/12] media: mt9m001: switch soc_mt9m001 to a standard subdev sensor driver Akinobu Mita
                   ` (7 preceding siblings ...)
  2018-12-22 17:12 ` [PATCH 08/12] media: mt9m001: add media controller support Akinobu Mita
@ 2018-12-22 17:12 ` Akinobu Mita
  2019-01-07 11:27   ` Sakari Ailus
  2018-12-22 17:12 ` [PATCH 10/12] media: mt9m001: support log_status ioctl and event interface Akinobu Mita
                   ` (3 subsequent siblings)
  12 siblings, 1 reply; 31+ messages in thread
From: Akinobu Mita @ 2018-12-22 17:12 UTC (permalink / raw)
  To: linux-media, devicetree
  Cc: Akinobu Mita, Guennadi Liakhovetski, Sakari Ailus, Mauro Carvalho Chehab

Register a sub-device to the asynchronous subdevice framework, and also
create subdevice device node.

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/Kconfig   | 2 +-
 drivers/media/i2c/mt9m001.c | 9 ++++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 5e30ad3..a6d8416 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -845,7 +845,7 @@ config VIDEO_VS6624
 
 config VIDEO_MT9M001
 	tristate "mt9m001 support"
-	depends on I2C && VIDEO_V4L2
+	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
 	depends on MEDIA_CAMERA_SUPPORT
 	depends on MEDIA_CONTROLLER
 	help
diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
index e31fb7d..b4deec3 100644
--- a/drivers/media/i2c/mt9m001.c
+++ b/drivers/media/i2c/mt9m001.c
@@ -716,6 +716,7 @@ static int mt9m001_probe(struct i2c_client *client,
 		return PTR_ERR(mt9m001->reset_gpio);
 
 	v4l2_i2c_subdev_init(&mt9m001->subdev, client, &mt9m001_subdev_ops);
+	mt9m001->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
 	v4l2_ctrl_handler_init(&mt9m001->hdl, 4);
 	v4l2_ctrl_new_std(&mt9m001->hdl, &mt9m001_ctrl_ops,
 			V4L2_CID_VFLIP, 0, 1, 1, 0);
@@ -765,10 +766,16 @@ static int mt9m001_probe(struct i2c_client *client,
 	if (ret)
 		goto error_power_off;
 
+	ret = v4l2_async_register_subdev(&mt9m001->subdev);
+	if (ret)
+		goto error_entity_cleanup;
+
 	pm_runtime_put_sync(&client->dev);
 
 	return 0;
 
+error_entity_cleanup:
+	media_entity_cleanup(&mt9m001->subdev.entity);
 error_power_off:
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
@@ -785,9 +792,9 @@ static int mt9m001_remove(struct i2c_client *client)
 {
 	struct mt9m001 *mt9m001 = to_mt9m001(client);
 
-	v4l2_device_unregister_subdev(&mt9m001->subdev);
 	pm_runtime_get_sync(&client->dev);
 
+	v4l2_async_unregister_subdev(&mt9m001->subdev);
 	media_entity_cleanup(&mt9m001->subdev.entity);
 
 	pm_runtime_disable(&client->dev);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 10/12] media: mt9m001: support log_status ioctl and event interface
  2018-12-22 17:12 [PATCH 00/12] media: mt9m001: switch soc_mt9m001 to a standard subdev sensor driver Akinobu Mita
                   ` (8 preceding siblings ...)
  2018-12-22 17:12 ` [PATCH 09/12] media: mt9m001: register to V4L2 asynchronous subdevice framework Akinobu Mita
@ 2018-12-22 17:12 ` Akinobu Mita
  2018-12-22 17:12 ` [PATCH 11/12] media: mt9m001: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY Akinobu Mita
                   ` (2 subsequent siblings)
  12 siblings, 0 replies; 31+ messages in thread
From: Akinobu Mita @ 2018-12-22 17:12 UTC (permalink / raw)
  To: linux-media, devicetree
  Cc: Akinobu Mita, Guennadi Liakhovetski, Sakari Ailus, Mauro Carvalho Chehab

This adds log_status ioctl and event interface for mt9m001's v4l2 controls.

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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
index b4deec3..a5b94d7 100644
--- a/drivers/media/i2c/mt9m001.c
+++ b/drivers/media/i2c/mt9m001.c
@@ -18,6 +18,7 @@
 #include <media/v4l2-subdev.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
+#include <media/v4l2-event.h>
 
 /*
  * mt9m001 i2c address 0x5d
@@ -628,6 +629,9 @@ static const struct v4l2_ctrl_ops mt9m001_ctrl_ops = {
 };
 
 static const struct v4l2_subdev_core_ops mt9m001_subdev_core_ops = {
+	.log_status = v4l2_ctrl_subdev_log_status,
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	.g_register	= mt9m001_g_register,
 	.s_register	= mt9m001_s_register,
@@ -716,7 +720,8 @@ static int mt9m001_probe(struct i2c_client *client,
 		return PTR_ERR(mt9m001->reset_gpio);
 
 	v4l2_i2c_subdev_init(&mt9m001->subdev, client, &mt9m001_subdev_ops);
-	mt9m001->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
+	mt9m001->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE |
+				V4L2_SUBDEV_FL_HAS_EVENTS;
 	v4l2_ctrl_handler_init(&mt9m001->hdl, 4);
 	v4l2_ctrl_new_std(&mt9m001->hdl, &mt9m001_ctrl_ops,
 			V4L2_CID_VFLIP, 0, 1, 1, 0);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 11/12] media: mt9m001: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY
  2018-12-22 17:12 [PATCH 00/12] media: mt9m001: switch soc_mt9m001 to a standard subdev sensor driver Akinobu Mita
                   ` (9 preceding siblings ...)
  2018-12-22 17:12 ` [PATCH 10/12] media: mt9m001: support log_status ioctl and event interface Akinobu Mita
@ 2018-12-22 17:12 ` Akinobu Mita
  2019-01-07 11:30   ` Sakari Ailus
  2018-12-22 17:12 ` [PATCH 12/12] media: mt9m001: set all mbus format field when G_FMT and S_FMT ioctls Akinobu Mita
  2019-01-07  9:36 ` [PATCH 00/12] media: mt9m001: switch soc_mt9m001 to a standard subdev sensor driver Sakari Ailus
  12 siblings, 1 reply; 31+ messages in thread
From: Akinobu Mita @ 2018-12-22 17:12 UTC (permalink / raw)
  To: linux-media, devicetree
  Cc: Akinobu Mita, Guennadi Liakhovetski, Sakari Ailus, Mauro Carvalho Chehab

The VIDIOC_SUBDEV_G_FMT ioctl for this driver doesn't recognize
V4L2_SUBDEV_FORMAT_TRY and always works as if V4L2_SUBDEV_FORMAT_ACTIVE
is specified.

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 | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
index a5b94d7..f4afbc9 100644
--- a/drivers/media/i2c/mt9m001.c
+++ b/drivers/media/i2c/mt9m001.c
@@ -331,6 +331,12 @@ static int mt9m001_get_fmt(struct v4l2_subdev *sd,
 	if (format->pad)
 		return -EINVAL;
 
+	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
+		mf = v4l2_subdev_get_try_format(sd, cfg, 0);
+		format->format = *mf;
+		return 0;
+	}
+
 	mf->width	= mt9m001->rect.width;
 	mf->height	= mt9m001->rect.height;
 	mf->code	= mt9m001->fmt->code;
@@ -638,6 +644,26 @@ static const struct v4l2_subdev_core_ops mt9m001_subdev_core_ops = {
 #endif
 };
 
+static int mt9m001_init_cfg(struct v4l2_subdev *sd,
+			    struct v4l2_subdev_pad_config *cfg)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct mt9m001 *mt9m001 = to_mt9m001(client);
+	struct v4l2_mbus_framefmt *try_fmt =
+		v4l2_subdev_get_try_format(sd, cfg, 0);
+
+	try_fmt->width		= mt9m001->rect.width;
+	try_fmt->height		= mt9m001->rect.height;
+	try_fmt->code		= mt9m001->fmt->code;
+	try_fmt->colorspace	= mt9m001->fmt->colorspace;
+	try_fmt->field		= V4L2_FIELD_NONE;
+	try_fmt->ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT;
+	try_fmt->quantization	= V4L2_QUANTIZATION_DEFAULT;
+	try_fmt->xfer_func	= V4L2_XFER_FUNC_DEFAULT;
+
+	return 0;
+}
+
 static int mt9m001_enum_mbus_code(struct v4l2_subdev *sd,
 		struct v4l2_subdev_pad_config *cfg,
 		struct v4l2_subdev_mbus_code_enum *code)
@@ -674,6 +700,7 @@ static const struct v4l2_subdev_sensor_ops mt9m001_subdev_sensor_ops = {
 };
 
 static const struct v4l2_subdev_pad_ops mt9m001_subdev_pad_ops = {
+	.init_cfg	= mt9m001_init_cfg,
 	.enum_mbus_code = mt9m001_enum_mbus_code,
 	.get_selection	= mt9m001_get_selection,
 	.set_selection	= mt9m001_set_selection,
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 12/12] media: mt9m001: set all mbus format field when G_FMT and S_FMT ioctls
  2018-12-22 17:12 [PATCH 00/12] media: mt9m001: switch soc_mt9m001 to a standard subdev sensor driver Akinobu Mita
                   ` (10 preceding siblings ...)
  2018-12-22 17:12 ` [PATCH 11/12] media: mt9m001: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY Akinobu Mita
@ 2018-12-22 17:12 ` Akinobu Mita
  2019-01-07 11:32   ` Sakari Ailus
  2019-01-07  9:36 ` [PATCH 00/12] media: mt9m001: switch soc_mt9m001 to a standard subdev sensor driver Sakari Ailus
  12 siblings, 1 reply; 31+ messages in thread
From: Akinobu Mita @ 2018-12-22 17:12 UTC (permalink / raw)
  To: linux-media, devicetree
  Cc: Akinobu Mita, Guennadi Liakhovetski, Sakari Ailus, Mauro Carvalho Chehab

This driver doesn't set all members of mbus format field when the
VIDIOC_SUBDEV_{S,G}_FMT ioctls are called.

This is detected by v4l2-compliance.

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 | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
index f4afbc9..82b89d5 100644
--- a/drivers/media/i2c/mt9m001.c
+++ b/drivers/media/i2c/mt9m001.c
@@ -342,6 +342,9 @@ static int mt9m001_get_fmt(struct v4l2_subdev *sd,
 	mf->code	= mt9m001->fmt->code;
 	mf->colorspace	= mt9m001->fmt->colorspace;
 	mf->field	= V4L2_FIELD_NONE;
+	mf->ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT;
+	mf->quantization = V4L2_QUANTIZATION_DEFAULT;
+	mf->xfer_func	= V4L2_XFER_FUNC_DEFAULT;
 
 	return 0;
 }
@@ -402,6 +405,10 @@ static int mt9m001_set_fmt(struct v4l2_subdev *sd,
 	}
 
 	mf->colorspace	= fmt->colorspace;
+	mf->field	= V4L2_FIELD_NONE;
+	mf->ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT;
+	mf->quantization = V4L2_QUANTIZATION_DEFAULT;
+	mf->xfer_func	= V4L2_XFER_FUNC_DEFAULT;
 
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
 		return mt9m001_s_fmt(sd, fmt, mf);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH 02/12] media: i2c: mt9m001: dt: add binding for mt9m001
  2018-12-22 17:12 ` [PATCH 02/12] media: i2c: mt9m001: dt: add binding for mt9m001 Akinobu Mita
@ 2019-01-03 21:12   ` Rob Herring
  2019-01-05 15:09     ` Akinobu Mita
  0 siblings, 1 reply; 31+ messages in thread
From: Rob Herring @ 2019-01-03 21:12 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-media, devicetree, Guennadi Liakhovetski, Sakari Ailus,
	Mauro Carvalho Chehab

On Sun, Dec 23, 2018 at 02:12:44AM +0900, Akinobu Mita wrote:
> Add device tree binding documentation for the MT9M001 CMOS image sensor.
> 
> Cc: Rob Herring <robh@kernel.org>
> 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>
> ---
>  .../devicetree/bindings/media/i2c/mt9m001.txt      | 37 ++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/mt9m001.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/mt9m001.txt b/Documentation/devicetree/bindings/media/i2c/mt9m001.txt
> new file mode 100644
> index 0000000..794b787
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/mt9m001.txt
> @@ -0,0 +1,37 @@
> +MT9M001: 1/2-Inch Megapixel Digital Image Sensor
> +
> +The MT9M001 is an SXGA-format with a 1/2-inch CMOS active-pixel digital
> +image sensor. It is programmable through a simple two-wire serial
> +interface.

I2C?

> +
> +Required Properties:
> +
> +- compatible: shall be "onnn,mt9m001".
> +- clocks: reference to the master clock into sensor
> +
> +Optional Properties:
> +
> +- reset-gpios: GPIO handle which is connected to the reset pin of the chip.
> +  Active low.
> +- standby-gpios: GPIO handle which is connected to the standby pin of the chip.
> +  Active high.
> +
> +For further reading on port node refer to
> +Documentation/devicetree/bindings/media/video-interfaces.txt.

You still need to state how many ports/endpoints and what they are.

> +
> +Example:
> +
> +	&i2c1 {
> +		mt9m001@5d {

camera-sensor@5d

> +			compatible = "onnn,mt9m001";
> +			reg = <0x5d>;
> +			reset-gpios = <&gpio0 0 GPIO_ACTIVE_LOW>;
> +			standby-gpios = <&gpio0 1 GPIO_ACTIVE_HIGH>;
> +			clocks = <&camera_clk>;
> +			port {
> +				mt9m001_out: endpoint {
> +					remote-endpoint = <&vcap_in>;
> +				};
> +			};
> +		};
> +	};
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 02/12] media: i2c: mt9m001: dt: add binding for mt9m001
  2019-01-03 21:12   ` Rob Herring
@ 2019-01-05 15:09     ` Akinobu Mita
  0 siblings, 0 replies; 31+ messages in thread
From: Akinobu Mita @ 2019-01-05 15:09 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-media, open list:OPEN FIRMWARE AND...,
	Guennadi Liakhovetski, Sakari Ailus, Mauro Carvalho Chehab

2019年1月4日(金) 6:12 Rob Herring <robh@kernel.org>:
>
> On Sun, Dec 23, 2018 at 02:12:44AM +0900, Akinobu Mita wrote:
> > Add device tree binding documentation for the MT9M001 CMOS image sensor.
> >
> > Cc: Rob Herring <robh@kernel.org>
> > 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>
> > ---
> >  .../devicetree/bindings/media/i2c/mt9m001.txt      | 37 ++++++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/mt9m001.txt
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/mt9m001.txt b/Documentation/devicetree/bindings/media/i2c/mt9m001.txt
> > new file mode 100644
> > index 0000000..794b787
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/mt9m001.txt
> > @@ -0,0 +1,37 @@
> > +MT9M001: 1/2-Inch Megapixel Digital Image Sensor
> > +
> > +The MT9M001 is an SXGA-format with a 1/2-inch CMOS active-pixel digital
> > +image sensor. It is programmable through a simple two-wire serial
> > +interface.
>
> I2C?

Sounds good.

> > +
> > +Required Properties:
> > +
> > +- compatible: shall be "onnn,mt9m001".
> > +- clocks: reference to the master clock into sensor
> > +
> > +Optional Properties:
> > +
> > +- reset-gpios: GPIO handle which is connected to the reset pin of the chip.
> > +  Active low.
> > +- standby-gpios: GPIO handle which is connected to the standby pin of the chip.
> > +  Active high.
> > +
> > +For further reading on port node refer to
> > +Documentation/devicetree/bindings/media/video-interfaces.txt.
>
> You still need to state how many ports/endpoints and what they are.

I'll write the following description that is copied from
Documentation/devicetree/bindings/media/i2c/mt9m111.txt.

"The device node must contain one 'port' child node with one 'endpoint' child
sub-node for its digital output video port, in accordance with the video
interface bindings defined in:
Documentation/devicetree/bindings/media/video-interfaces.txt"

> > +
> > +Example:
> > +
> > +     &i2c1 {
> > +             mt9m001@5d {
>
> camera-sensor@5d

OK.

> > +                     compatible = "onnn,mt9m001";
> > +                     reg = <0x5d>;
> > +                     reset-gpios = <&gpio0 0 GPIO_ACTIVE_LOW>;
> > +                     standby-gpios = <&gpio0 1 GPIO_ACTIVE_HIGH>;
> > +                     clocks = <&camera_clk>;
> > +                     port {
> > +                             mt9m001_out: endpoint {
> > +                                     remote-endpoint = <&vcap_in>;
> > +                             };
> > +                     };
> > +             };
> > +     };
> > --
> > 2.7.4
> >

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 00/12] media: mt9m001: switch soc_mt9m001 to a standard subdev sensor driver
  2018-12-22 17:12 [PATCH 00/12] media: mt9m001: switch soc_mt9m001 to a standard subdev sensor driver Akinobu Mita
                   ` (11 preceding siblings ...)
  2018-12-22 17:12 ` [PATCH 12/12] media: mt9m001: set all mbus format field when G_FMT and S_FMT ioctls Akinobu Mita
@ 2019-01-07  9:36 ` Sakari Ailus
  2019-01-07  9:48   ` Sakari Ailus
  12 siblings, 1 reply; 31+ messages in thread
From: Sakari Ailus @ 2019-01-07  9:36 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-media, devicetree, Rob Herring, Guennadi Liakhovetski,
	Mauro Carvalho Chehab

Hi Mita-san,

On Sun, Dec 23, 2018 at 02:12:42AM +0900, Akinobu Mita wrote:
> This patchset converts soc_camera mt9m001 driver to a standard subdev
> sensor driver.
> 
> Akinobu Mita (12):
>   media: i2c: mt9m001: copy mt9m001 soc_camera sensor driver
>   media: i2c: mt9m001: dt: add binding for mt9m001
>   media: mt9m001: convert to SPDX license identifer
>   media: mt9m001: add of_match_table
>   media: mt9m001: introduce multi_reg_write()
>   media: mt9m001: switch s_power callback to runtime PM
>   media: mt9m001: remove remaining soc_camera specific code
>   media: mt9m001: add media controller support
>   media: mt9m001: register to V4L2 asynchronous subdevice framework
>   media: mt9m001: support log_status ioctl and event interface
>   media: mt9m001: make VIDIOC_SUBDEV_G_FMT ioctl work with
>     V4L2_SUBDEV_FORMAT_TRY
>   media: mt9m001: set all mbus format field when G_FMT and S_FMT ioctls
> 
>  .../devicetree/bindings/media/i2c/mt9m001.txt      |  37 +
>  drivers/media/i2c/Kconfig                          |   9 +
>  drivers/media/i2c/Makefile                         |   1 +
>  drivers/media/i2c/mt9m001.c                        | 900 +++++++++++++++++++++
>  4 files changed, 947 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/mt9m001.txt
>  create mode 100644 drivers/media/i2c/mt9m001.c
> 
> Cc: Rob Herring <robh@kernel.org>
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>

There's something wrong somewhere --- I can't see the set on the LMML.
Could you resend it, please (or v2 to address Rob's comments)? I don't
think the number of recipients is too big, so I hope this was a transient
problem somehere that shouldn't have happened. Hmm.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 00/12] media: mt9m001: switch soc_mt9m001 to a standard subdev sensor driver
  2019-01-07  9:36 ` [PATCH 00/12] media: mt9m001: switch soc_mt9m001 to a standard subdev sensor driver Sakari Ailus
@ 2019-01-07  9:48   ` Sakari Ailus
  0 siblings, 0 replies; 31+ messages in thread
From: Sakari Ailus @ 2019-01-07  9:48 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-media, devicetree, Rob Herring, Guennadi Liakhovetski,
	Mauro Carvalho Chehab

On Mon, Jan 07, 2019 at 11:36:03AM +0200, Sakari Ailus wrote:
> Hi Mita-san,
> 
> On Sun, Dec 23, 2018 at 02:12:42AM +0900, Akinobu Mita wrote:
> > This patchset converts soc_camera mt9m001 driver to a standard subdev
> > sensor driver.
> > 
> > Akinobu Mita (12):
> >   media: i2c: mt9m001: copy mt9m001 soc_camera sensor driver
> >   media: i2c: mt9m001: dt: add binding for mt9m001
> >   media: mt9m001: convert to SPDX license identifer
> >   media: mt9m001: add of_match_table
> >   media: mt9m001: introduce multi_reg_write()
> >   media: mt9m001: switch s_power callback to runtime PM
> >   media: mt9m001: remove remaining soc_camera specific code
> >   media: mt9m001: add media controller support
> >   media: mt9m001: register to V4L2 asynchronous subdevice framework
> >   media: mt9m001: support log_status ioctl and event interface
> >   media: mt9m001: make VIDIOC_SUBDEV_G_FMT ioctl work with
> >     V4L2_SUBDEV_FORMAT_TRY
> >   media: mt9m001: set all mbus format field when G_FMT and S_FMT ioctls
> > 
> >  .../devicetree/bindings/media/i2c/mt9m001.txt      |  37 +
> >  drivers/media/i2c/Kconfig                          |   9 +
> >  drivers/media/i2c/Makefile                         |   1 +
> >  drivers/media/i2c/mt9m001.c                        | 900 +++++++++++++++++++++
> >  4 files changed, 947 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/mt9m001.txt
> >  create mode 100644 drivers/media/i2c/mt9m001.c
> > 
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Cc: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> 
> There's something wrong somewhere --- I can't see the set on the LMML.
> Could you resend it, please (or v2 to address Rob's comments)? I don't
> think the number of recipients is too big, so I hope this was a transient
> problem somehere that shouldn't have happened. Hmm.

Oh well, I can see my own reply as the set. Please ignore.

-- 
Sakari Ailus

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 09/12] media: mt9m001: register to V4L2 asynchronous subdevice framework
  2018-12-22 17:12 ` [PATCH 09/12] media: mt9m001: register to V4L2 asynchronous subdevice framework Akinobu Mita
@ 2019-01-07 11:27   ` Sakari Ailus
  2019-01-07 14:09     ` Akinobu Mita
  0 siblings, 1 reply; 31+ messages in thread
From: Sakari Ailus @ 2019-01-07 11:27 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-media, devicetree, Guennadi Liakhovetski, Mauro Carvalho Chehab

Hi Mita-san,

On Sun, Dec 23, 2018 at 02:12:51AM +0900, Akinobu Mita wrote:
> Register a sub-device to the asynchronous subdevice framework, and also
> create subdevice device node.
> 
> 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/Kconfig   | 2 +-
>  drivers/media/i2c/mt9m001.c | 9 ++++++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 5e30ad3..a6d8416 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -845,7 +845,7 @@ config VIDEO_VS6624
>  
>  config VIDEO_MT9M001
>  	tristate "mt9m001 support"
> -	depends on I2C && VIDEO_V4L2
> +	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API

VIDEO_V4L2_SUBDEV_API depends on MEDIA_CONTROLLER, so MEDIA_CONTROLLER
below can be removed.

>  	depends on MEDIA_CAMERA_SUPPORT
>  	depends on MEDIA_CONTROLLER
>  	help
> diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
> index e31fb7d..b4deec3 100644
> --- a/drivers/media/i2c/mt9m001.c
> +++ b/drivers/media/i2c/mt9m001.c
> @@ -716,6 +716,7 @@ static int mt9m001_probe(struct i2c_client *client,
>  		return PTR_ERR(mt9m001->reset_gpio);
>  
>  	v4l2_i2c_subdev_init(&mt9m001->subdev, client, &mt9m001_subdev_ops);
> +	mt9m001->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;

|=

Otherwise you lose flags set by v4l2_i2c_subdev_init().

>  	v4l2_ctrl_handler_init(&mt9m001->hdl, 4);
>  	v4l2_ctrl_new_std(&mt9m001->hdl, &mt9m001_ctrl_ops,
>  			V4L2_CID_VFLIP, 0, 1, 1, 0);
> @@ -765,10 +766,16 @@ static int mt9m001_probe(struct i2c_client *client,
>  	if (ret)
>  		goto error_power_off;
>  
> +	ret = v4l2_async_register_subdev(&mt9m001->subdev);
> +	if (ret)
> +		goto error_entity_cleanup;
> +
>  	pm_runtime_put_sync(&client->dev);
>  
>  	return 0;
>  
> +error_entity_cleanup:
> +	media_entity_cleanup(&mt9m001->subdev.entity);
>  error_power_off:
>  	pm_runtime_disable(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
> @@ -785,9 +792,9 @@ static int mt9m001_remove(struct i2c_client *client)
>  {
>  	struct mt9m001 *mt9m001 = to_mt9m001(client);
>  
> -	v4l2_device_unregister_subdev(&mt9m001->subdev);
>  	pm_runtime_get_sync(&client->dev);
>  
> +	v4l2_async_unregister_subdev(&mt9m001->subdev);
>  	media_entity_cleanup(&mt9m001->subdev.entity);
>  
>  	pm_runtime_disable(&client->dev);

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 07/12] media: mt9m001: remove remaining soc_camera specific code
  2018-12-22 17:12 ` [PATCH 07/12] media: mt9m001: remove remaining soc_camera specific code Akinobu Mita
@ 2019-01-07 11:29   ` Sakari Ailus
  2019-01-07 14:13     ` Akinobu Mita
  0 siblings, 1 reply; 31+ messages in thread
From: Sakari Ailus @ 2019-01-07 11:29 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-media, devicetree, Guennadi Liakhovetski, Mauro Carvalho Chehab

Hi Mita-san,

On Sun, Dec 23, 2018 at 02:12:49AM +0900, Akinobu Mita wrote:
> Remove remaining soc_camera specific code and drop soc_camera dependency
> from this driver.
> 
> 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/Kconfig   |  2 +-
>  drivers/media/i2c/mt9m001.c | 84 ++++++++-------------------------------------
>  2 files changed, 15 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 0efc038..4bdf043 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -845,7 +845,7 @@ config VIDEO_VS6624
>  
>  config VIDEO_MT9M001
>  	tristate "mt9m001 support"
> -	depends on SOC_CAMERA && I2C
> +	depends on I2C && VIDEO_V4L2
>  	help
>  	  This driver supports MT9M001 cameras from Micron, monochrome
>  	  and colour models.
> diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
> index f20188a..eb5c4ed 100644
> --- a/drivers/media/i2c/mt9m001.c
> +++ b/drivers/media/i2c/mt9m001.c
> @@ -15,15 +15,12 @@
>  #include <linux/log2.h>
>  #include <linux/module.h>
>  
> -#include <media/soc_camera.h>
> -#include <media/drv-intf/soc_mediabus.h>
>  #include <media/v4l2-subdev.h>
>  #include <media/v4l2-ctrls.h>

While at it, could you order the list alphabetically? The same applies to
further patches changing included files.

> +#include <media/v4l2-device.h>
>  
>  /*
>   * mt9m001 i2c address 0x5d
> - * The platform has to define struct i2c_board_info objects and link to them
> - * from struct soc_camera_host_desc
>   */
>  
>  /* mt9m001 selected register addresses */
> @@ -278,11 +275,15 @@ static int mt9m001_set_selection(struct v4l2_subdev *sd,
>  	rect.width = ALIGN(rect.width, 2);
>  	rect.left = ALIGN(rect.left, 2);
>  
> -	soc_camera_limit_side(&rect.left, &rect.width,
> -		     MT9M001_COLUMN_SKIP, MT9M001_MIN_WIDTH, MT9M001_MAX_WIDTH);
> +	rect.width = clamp_t(u32, rect.width, MT9M001_MIN_WIDTH,
> +			MT9M001_MAX_WIDTH);
> +	rect.left = clamp_t(u32, rect.left, MT9M001_COLUMN_SKIP,
> +			MT9M001_COLUMN_SKIP + MT9M001_MAX_WIDTH - rect.width);
>  
> -	soc_camera_limit_side(&rect.top, &rect.height,
> -		     MT9M001_ROW_SKIP, MT9M001_MIN_HEIGHT, MT9M001_MAX_HEIGHT);
> +	rect.height = clamp_t(u32, rect.height, MT9M001_MIN_HEIGHT,
> +			MT9M001_MAX_HEIGHT);
> +	rect.top = clamp_t(u32, rect.top, MT9M001_ROW_SKIP,
> +			MT9M001_ROW_SKIP + MT9M001_MAX_HEIGHT - rect.width);
>  
>  	mt9m001->total_h = rect.height + mt9m001->y_skip_top +
>  			   MT9M001_DEFAULT_VBLANK;
> @@ -561,12 +562,10 @@ static int mt9m001_s_ctrl(struct v4l2_ctrl *ctrl)
>   * Interface active, can use i2c. If it fails, it can indeed mean, that
>   * this wasn't our capture interface, so, we wait for the right one
>   */
> -static int mt9m001_video_probe(struct soc_camera_subdev_desc *ssdd,
> -			       struct i2c_client *client)
> +static int mt9m001_video_probe(struct i2c_client *client)
>  {
>  	struct mt9m001 *mt9m001 = to_mt9m001(client);
>  	s32 data;
> -	unsigned long flags;
>  	int ret;
>  
>  	/* Enable the chip */
> @@ -581,9 +580,11 @@ static int mt9m001_video_probe(struct soc_camera_subdev_desc *ssdd,
>  	case 0x8411:
>  	case 0x8421:
>  		mt9m001->fmts = mt9m001_colour_fmts;
> +		mt9m001->num_fmts = ARRAY_SIZE(mt9m001_colour_fmts);
>  		break;
>  	case 0x8431:
>  		mt9m001->fmts = mt9m001_monochrome_fmts;
> +		mt9m001->num_fmts = ARRAY_SIZE(mt9m001_monochrome_fmts);
>  		break;
>  	default:
>  		dev_err(&client->dev,
> @@ -592,26 +593,6 @@ static int mt9m001_video_probe(struct soc_camera_subdev_desc *ssdd,
>  		goto done;
>  	}
>  
> -	mt9m001->num_fmts = 0;
> -
> -	/*
> -	 * This is a 10bit sensor, so by default we only allow 10bit.
> -	 * The platform may support different bus widths due to
> -	 * different routing of the data lines.
> -	 */
> -	if (ssdd->query_bus_param)
> -		flags = ssdd->query_bus_param(ssdd);
> -	else
> -		flags = SOCAM_DATAWIDTH_10;
> -
> -	if (flags & SOCAM_DATAWIDTH_10)
> -		mt9m001->num_fmts++;
> -	else
> -		mt9m001->fmts++;
> -
> -	if (flags & SOCAM_DATAWIDTH_8)
> -		mt9m001->num_fmts++;
> -
>  	mt9m001->fmt = &mt9m001->fmts[0];
>  
>  	dev_info(&client->dev, "Detected a MT9M001 chip ID %x (%s)\n", data,
> @@ -630,12 +611,6 @@ static int mt9m001_video_probe(struct soc_camera_subdev_desc *ssdd,
>  	return ret;
>  }
>  
> -static void mt9m001_video_remove(struct soc_camera_subdev_desc *ssdd)
> -{
> -	if (ssdd->free_bus)
> -		ssdd->free_bus(ssdd);
> -}
> -
>  static int mt9m001_g_skip_top_lines(struct v4l2_subdev *sd, u32 *lines)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> @@ -675,41 +650,18 @@ static int mt9m001_enum_mbus_code(struct v4l2_subdev *sd,
>  static int mt9m001_g_mbus_config(struct v4l2_subdev *sd,
>  				struct v4l2_mbus_config *cfg)
>  {
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
> -	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
> -
>  	/* MT9M001 has all capture_format parameters fixed */
>  	cfg->flags = V4L2_MBUS_PCLK_SAMPLE_FALLING |
>  		V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_HIGH |
>  		V4L2_MBUS_DATA_ACTIVE_HIGH | V4L2_MBUS_MASTER;
>  	cfg->type = V4L2_MBUS_PARALLEL;
> -	cfg->flags = soc_camera_apply_board_flags(ssdd, cfg);
>  
>  	return 0;
>  }
>  
> -static int mt9m001_s_mbus_config(struct v4l2_subdev *sd,
> -				const struct v4l2_mbus_config *cfg)
> -{
> -	const 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);
> -	unsigned int bps = soc_mbus_get_fmtdesc(mt9m001->fmt->code)->bits_per_sample;
> -
> -	if (ssdd->set_bus_param)
> -		return ssdd->set_bus_param(ssdd, 1 << (bps - 1));
> -
> -	/*
> -	 * Without board specific bus width settings we only support the
> -	 * sensors native bus width
> -	 */
> -	return bps == 10 ? 0 : -EINVAL;
> -}
> -
>  static const struct v4l2_subdev_video_ops mt9m001_subdev_video_ops = {
>  	.s_stream	= mt9m001_s_stream,
>  	.g_mbus_config	= mt9m001_g_mbus_config,
> -	.s_mbus_config	= mt9m001_s_mbus_config,
>  };
>  
>  static const struct v4l2_subdev_sensor_ops mt9m001_subdev_sensor_ops = {
> @@ -736,21 +688,15 @@ static int mt9m001_probe(struct i2c_client *client,
>  {
>  	struct mt9m001 *mt9m001;
>  	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> -	struct soc_camera_subdev_desc *ssdd = soc_camera_i2c_to_desc(client);
>  	int ret;
>  
> -	if (!ssdd) {
> -		dev_err(&client->dev, "MT9M001 driver needs platform data\n");
> -		return -EINVAL;
> -	}
> -
>  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
>  		dev_warn(&adapter->dev,
>  			 "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
>  		return -EIO;
>  	}
>  
> -	mt9m001 = devm_kzalloc(&client->dev, sizeof(struct mt9m001), GFP_KERNEL);
> +	mt9m001 = devm_kzalloc(&client->dev, sizeof(*mt9m001), GFP_KERNEL);
>  	if (!mt9m001)
>  		return -ENOMEM;
>  
> @@ -808,7 +754,7 @@ static int mt9m001_probe(struct i2c_client *client,
>  	pm_runtime_set_active(&client->dev);
>  	pm_runtime_enable(&client->dev);
>  
> -	ret = mt9m001_video_probe(ssdd, client);
> +	ret = mt9m001_video_probe(client);
>  	if (ret)
>  		goto error_power_off;
>  
> @@ -831,7 +777,6 @@ static int mt9m001_probe(struct i2c_client *client,
>  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_device_unregister_subdev(&mt9m001->subdev);
>  	pm_runtime_get_sync(&client->dev);
> @@ -842,7 +787,6 @@ static int mt9m001_remove(struct i2c_client *client)
>  	mt9m001_power_off(mt9m001);
>  
>  	v4l2_ctrl_handler_free(&mt9m001->hdl);
> -	mt9m001_video_remove(ssdd);
>  	mutex_destroy(&mt9m001->mutex);
>  
>  	return 0;
> -- 
> 2.7.4
> 

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 11/12] media: mt9m001: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY
  2018-12-22 17:12 ` [PATCH 11/12] media: mt9m001: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY Akinobu Mita
@ 2019-01-07 11:30   ` Sakari Ailus
  2019-01-07 14:15     ` Akinobu Mita
  0 siblings, 1 reply; 31+ messages in thread
From: Sakari Ailus @ 2019-01-07 11:30 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-media, devicetree, Guennadi Liakhovetski, Mauro Carvalho Chehab

Hi Mita-san,

On Sun, Dec 23, 2018 at 02:12:53AM +0900, Akinobu Mita wrote:
> The VIDIOC_SUBDEV_G_FMT ioctl for this driver doesn't recognize
> V4L2_SUBDEV_FORMAT_TRY and always works as if V4L2_SUBDEV_FORMAT_ACTIVE
> is specified.
> 
> 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 | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
> index a5b94d7..f4afbc9 100644
> --- a/drivers/media/i2c/mt9m001.c
> +++ b/drivers/media/i2c/mt9m001.c
> @@ -331,6 +331,12 @@ static int mt9m001_get_fmt(struct v4l2_subdev *sd,
>  	if (format->pad)
>  		return -EINVAL;
>  
> +	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> +		mf = v4l2_subdev_get_try_format(sd, cfg, 0);
> +		format->format = *mf;
> +		return 0;
> +	}
> +
>  	mf->width	= mt9m001->rect.width;
>  	mf->height	= mt9m001->rect.height;
>  	mf->code	= mt9m001->fmt->code;
> @@ -638,6 +644,26 @@ static const struct v4l2_subdev_core_ops mt9m001_subdev_core_ops = {
>  #endif
>  };
>  
> +static int mt9m001_init_cfg(struct v4l2_subdev *sd,
> +			    struct v4l2_subdev_pad_config *cfg)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct mt9m001 *mt9m001 = to_mt9m001(client);
> +	struct v4l2_mbus_framefmt *try_fmt =
> +		v4l2_subdev_get_try_format(sd, cfg, 0);
> +
> +	try_fmt->width		= mt9m001->rect.width;
> +	try_fmt->height		= mt9m001->rect.height;
> +	try_fmt->code		= mt9m001->fmt->code;
> +	try_fmt->colorspace	= mt9m001->fmt->colorspace;

The initial configuration set here should reflect the default, not current
configuration. This appears to refer to the current one.

> +	try_fmt->field		= V4L2_FIELD_NONE;
> +	try_fmt->ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT;
> +	try_fmt->quantization	= V4L2_QUANTIZATION_DEFAULT;
> +	try_fmt->xfer_func	= V4L2_XFER_FUNC_DEFAULT;
> +
> +	return 0;
> +}
> +
>  static int mt9m001_enum_mbus_code(struct v4l2_subdev *sd,
>  		struct v4l2_subdev_pad_config *cfg,
>  		struct v4l2_subdev_mbus_code_enum *code)
> @@ -674,6 +700,7 @@ static const struct v4l2_subdev_sensor_ops mt9m001_subdev_sensor_ops = {
>  };
>  
>  static const struct v4l2_subdev_pad_ops mt9m001_subdev_pad_ops = {
> +	.init_cfg	= mt9m001_init_cfg,
>  	.enum_mbus_code = mt9m001_enum_mbus_code,
>  	.get_selection	= mt9m001_get_selection,
>  	.set_selection	= mt9m001_set_selection,
> -- 
> 2.7.4
> 

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 12/12] media: mt9m001: set all mbus format field when G_FMT and S_FMT ioctls
  2018-12-22 17:12 ` [PATCH 12/12] media: mt9m001: set all mbus format field when G_FMT and S_FMT ioctls Akinobu Mita
@ 2019-01-07 11:32   ` Sakari Ailus
  2019-01-07 14:18     ` Akinobu Mita
  0 siblings, 1 reply; 31+ messages in thread
From: Sakari Ailus @ 2019-01-07 11:32 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-media, devicetree, Guennadi Liakhovetski, Mauro Carvalho Chehab

Hi Mita-san,

On Sun, Dec 23, 2018 at 02:12:54AM +0900, Akinobu Mita wrote:
> This driver doesn't set all members of mbus format field when the
> VIDIOC_SUBDEV_{S,G}_FMT ioctls are called.
> 
> This is detected by v4l2-compliance.
> 
> 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 | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
> index f4afbc9..82b89d5 100644
> --- a/drivers/media/i2c/mt9m001.c
> +++ b/drivers/media/i2c/mt9m001.c
> @@ -342,6 +342,9 @@ static int mt9m001_get_fmt(struct v4l2_subdev *sd,
>  	mf->code	= mt9m001->fmt->code;
>  	mf->colorspace	= mt9m001->fmt->colorspace;
>  	mf->field	= V4L2_FIELD_NONE;
> +	mf->ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT;
> +	mf->quantization = V4L2_QUANTIZATION_DEFAULT;
> +	mf->xfer_func	= V4L2_XFER_FUNC_DEFAULT;

Instead of setting the fields individually, would it be feasible to just
assign mt9m001->fmt to mf?

>  
>  	return 0;
>  }
> @@ -402,6 +405,10 @@ static int mt9m001_set_fmt(struct v4l2_subdev *sd,
>  	}
>  
>  	mf->colorspace	= fmt->colorspace;
> +	mf->field	= V4L2_FIELD_NONE;
> +	mf->ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT;
> +	mf->quantization = V4L2_QUANTIZATION_DEFAULT;
> +	mf->xfer_func	= V4L2_XFER_FUNC_DEFAULT;

Ditto.

>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
>  		return mt9m001_s_fmt(sd, fmt, mf);

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 03/12] media: mt9m001: convert to SPDX license identifer
  2018-12-22 17:12 ` [PATCH 03/12] media: mt9m001: convert to SPDX license identifer Akinobu Mita
@ 2019-01-07 11:37   ` Sakari Ailus
  2019-01-07 14:20     ` Akinobu Mita
  0 siblings, 1 reply; 31+ messages in thread
From: Sakari Ailus @ 2019-01-07 11:37 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-media, devicetree, Guennadi Liakhovetski, Mauro Carvalho Chehab

Hi Mita-san,

On Sun, Dec 23, 2018 at 02:12:45AM +0900, Akinobu Mita wrote:
> Replace GPL license statements with SPDX license identifiers (GPL-2.0).
> 
> 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 | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
> index a1a85ff..65ff59d 100644
> --- a/drivers/media/i2c/mt9m001.c
> +++ b/drivers/media/i2c/mt9m001.c
> @@ -1,11 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   * Driver for MT9M001 CMOS Image Sensor from Micron
>   *
>   * Copyright (C) 2008, Guennadi Liakhovetski <kernel@pengutronix.de>
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
>   */
>  
>  #include <linux/videodev2.h>

The MODULE_LICENSE macro at the end of the file lists "GPL" as the license,
i.e. including later versions. I'm not sure what was the intention
originally. I guess it's safer to stick to 2.0, at least unless the
original author is able to shed some light into this.

Guennadi?

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 06/12] media: mt9m001: switch s_power callback to runtime PM
       [not found]   ` <20190107100034.po3jsnc3rdj37l4x@kekkonen.localdomain>
@ 2019-01-07 14:07     ` Akinobu Mita
  2019-01-07 14:10       ` Sakari Ailus
  0 siblings, 1 reply; 31+ messages in thread
From: Akinobu Mita @ 2019-01-07 14:07 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: linux-media, open list:OPEN FIRMWARE AND...,
	Guennadi Liakhovetski, Mauro Carvalho Chehab

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.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 09/12] media: mt9m001: register to V4L2 asynchronous subdevice framework
  2019-01-07 11:27   ` Sakari Ailus
@ 2019-01-07 14:09     ` Akinobu Mita
  0 siblings, 0 replies; 31+ messages in thread
From: Akinobu Mita @ 2019-01-07 14:09 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: linux-media, open list:OPEN FIRMWARE AND...,
	Guennadi Liakhovetski, Mauro Carvalho Chehab

2019年1月7日(月) 20:27 Sakari Ailus <sakari.ailus@linux.intel.com>:
>
> Hi Mita-san,
>
> On Sun, Dec 23, 2018 at 02:12:51AM +0900, Akinobu Mita wrote:
> > Register a sub-device to the asynchronous subdevice framework, and also
> > create subdevice device node.
> >
> > 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/Kconfig   | 2 +-
> >  drivers/media/i2c/mt9m001.c | 9 ++++++++-
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index 5e30ad3..a6d8416 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -845,7 +845,7 @@ config VIDEO_VS6624
> >
> >  config VIDEO_MT9M001
> >       tristate "mt9m001 support"
> > -     depends on I2C && VIDEO_V4L2
> > +     depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
>
> VIDEO_V4L2_SUBDEV_API depends on MEDIA_CONTROLLER, so MEDIA_CONTROLLER
> below can be removed.

OK.

> >       depends on MEDIA_CAMERA_SUPPORT
> >       depends on MEDIA_CONTROLLER
> >       help
> > diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
> > index e31fb7d..b4deec3 100644
> > --- a/drivers/media/i2c/mt9m001.c
> > +++ b/drivers/media/i2c/mt9m001.c
> > @@ -716,6 +716,7 @@ static int mt9m001_probe(struct i2c_client *client,
> >               return PTR_ERR(mt9m001->reset_gpio);
> >
> >       v4l2_i2c_subdev_init(&mt9m001->subdev, client, &mt9m001_subdev_ops);
> > +     mt9m001->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
>
> |=
>
> Otherwise you lose flags set by v4l2_i2c_subdev_init().

Oops.  Thanks for spotting.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 06/12] media: mt9m001: switch s_power callback to runtime PM
  2019-01-07 14:07     ` Akinobu Mita
@ 2019-01-07 14:10       ` Sakari Ailus
  0 siblings, 0 replies; 31+ messages in thread
From: Sakari Ailus @ 2019-01-07 14:10 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-media, open list:OPEN FIRMWARE AND...,
	Guennadi Liakhovetski, Mauro Carvalho Chehab

Hi Mita-san,

On Mon, Jan 07, 2019 at 11:07:18PM +0900, Akinobu Mita wrote:
> 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;
> }

I'd prefer the first; it's cleaner. I might call the new label e.g.
put_unlock as that describes what it does.

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 07/12] media: mt9m001: remove remaining soc_camera specific code
  2019-01-07 11:29   ` Sakari Ailus
@ 2019-01-07 14:13     ` Akinobu Mita
  0 siblings, 0 replies; 31+ messages in thread
From: Akinobu Mita @ 2019-01-07 14:13 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: linux-media, open list:OPEN FIRMWARE AND...,
	Guennadi Liakhovetski, Mauro Carvalho Chehab

2019年1月7日(月) 20:29 Sakari Ailus <sakari.ailus@linux.intel.com>:
>
> Hi Mita-san,
>
> On Sun, Dec 23, 2018 at 02:12:49AM +0900, Akinobu Mita wrote:
> > Remove remaining soc_camera specific code and drop soc_camera dependency
> > from this driver.
> >
> > 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/Kconfig   |  2 +-
> >  drivers/media/i2c/mt9m001.c | 84 ++++++++-------------------------------------
> >  2 files changed, 15 insertions(+), 71 deletions(-)
> >
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index 0efc038..4bdf043 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -845,7 +845,7 @@ config VIDEO_VS6624
> >
> >  config VIDEO_MT9M001
> >       tristate "mt9m001 support"
> > -     depends on SOC_CAMERA && I2C
> > +     depends on I2C && VIDEO_V4L2
> >       help
> >         This driver supports MT9M001 cameras from Micron, monochrome
> >         and colour models.
> > diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
> > index f20188a..eb5c4ed 100644
> > --- a/drivers/media/i2c/mt9m001.c
> > +++ b/drivers/media/i2c/mt9m001.c
> > @@ -15,15 +15,12 @@
> >  #include <linux/log2.h>
> >  #include <linux/module.h>
> >
> > -#include <media/soc_camera.h>
> > -#include <media/drv-intf/soc_mediabus.h>
> >  #include <media/v4l2-subdev.h>
> >  #include <media/v4l2-ctrls.h>
>
> While at it, could you order the list alphabetically? The same applies to
> further patches changing included files.

OK.  I'll prepare another separate patch.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 11/12] media: mt9m001: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY
  2019-01-07 11:30   ` Sakari Ailus
@ 2019-01-07 14:15     ` Akinobu Mita
  0 siblings, 0 replies; 31+ messages in thread
From: Akinobu Mita @ 2019-01-07 14:15 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: linux-media, open list:OPEN FIRMWARE AND...,
	Guennadi Liakhovetski, Mauro Carvalho Chehab

2019年1月7日(月) 20:30 Sakari Ailus <sakari.ailus@linux.intel.com>:
>
> Hi Mita-san,
>
> On Sun, Dec 23, 2018 at 02:12:53AM +0900, Akinobu Mita wrote:
> > The VIDIOC_SUBDEV_G_FMT ioctl for this driver doesn't recognize
> > V4L2_SUBDEV_FORMAT_TRY and always works as if V4L2_SUBDEV_FORMAT_ACTIVE
> > is specified.
> >
> > 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 | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
> > index a5b94d7..f4afbc9 100644
> > --- a/drivers/media/i2c/mt9m001.c
> > +++ b/drivers/media/i2c/mt9m001.c
> > @@ -331,6 +331,12 @@ static int mt9m001_get_fmt(struct v4l2_subdev *sd,
> >       if (format->pad)
> >               return -EINVAL;
> >
> > +     if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> > +             mf = v4l2_subdev_get_try_format(sd, cfg, 0);
> > +             format->format = *mf;
> > +             return 0;
> > +     }
> > +
> >       mf->width       = mt9m001->rect.width;
> >       mf->height      = mt9m001->rect.height;
> >       mf->code        = mt9m001->fmt->code;
> > @@ -638,6 +644,26 @@ static const struct v4l2_subdev_core_ops mt9m001_subdev_core_ops = {
> >  #endif
> >  };
> >
> > +static int mt9m001_init_cfg(struct v4l2_subdev *sd,
> > +                         struct v4l2_subdev_pad_config *cfg)
> > +{
> > +     struct i2c_client *client = v4l2_get_subdevdata(sd);
> > +     struct mt9m001 *mt9m001 = to_mt9m001(client);
> > +     struct v4l2_mbus_framefmt *try_fmt =
> > +             v4l2_subdev_get_try_format(sd, cfg, 0);
> > +
> > +     try_fmt->width          = mt9m001->rect.width;
> > +     try_fmt->height         = mt9m001->rect.height;
> > +     try_fmt->code           = mt9m001->fmt->code;
> > +     try_fmt->colorspace     = mt9m001->fmt->colorspace;
>
> The initial configuration set here should reflect the default, not current
> configuration. This appears to refer to the current one.

You are right.  I'll fix this.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 12/12] media: mt9m001: set all mbus format field when G_FMT and S_FMT ioctls
  2019-01-07 11:32   ` Sakari Ailus
@ 2019-01-07 14:18     ` Akinobu Mita
  2019-01-07 15:16       ` Sakari Ailus
  0 siblings, 1 reply; 31+ messages in thread
From: Akinobu Mita @ 2019-01-07 14:18 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: linux-media, open list:OPEN FIRMWARE AND...,
	Guennadi Liakhovetski, Mauro Carvalho Chehab

2019年1月7日(月) 20:32 Sakari Ailus <sakari.ailus@linux.intel.com>:
>
> Hi Mita-san,
>
> On Sun, Dec 23, 2018 at 02:12:54AM +0900, Akinobu Mita wrote:
> > This driver doesn't set all members of mbus format field when the
> > VIDIOC_SUBDEV_{S,G}_FMT ioctls are called.
> >
> > This is detected by v4l2-compliance.
> >
> > 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 | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
> > index f4afbc9..82b89d5 100644
> > --- a/drivers/media/i2c/mt9m001.c
> > +++ b/drivers/media/i2c/mt9m001.c
> > @@ -342,6 +342,9 @@ static int mt9m001_get_fmt(struct v4l2_subdev *sd,
> >       mf->code        = mt9m001->fmt->code;
> >       mf->colorspace  = mt9m001->fmt->colorspace;
> >       mf->field       = V4L2_FIELD_NONE;
> > +     mf->ycbcr_enc   = V4L2_YCBCR_ENC_DEFAULT;
> > +     mf->quantization = V4L2_QUANTIZATION_DEFAULT;
> > +     mf->xfer_func   = V4L2_XFER_FUNC_DEFAULT;
>
> Instead of setting the fields individually, would it be feasible to just
> assign mt9m001->fmt to mf?

The data type of mt9m001->fmt is struct mt9m001_datafmt, so it can't
simply assign to mf.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 03/12] media: mt9m001: convert to SPDX license identifer
  2019-01-07 11:37   ` Sakari Ailus
@ 2019-01-07 14:20     ` Akinobu Mita
  2019-01-07 15:15       ` Sakari Ailus
  0 siblings, 1 reply; 31+ messages in thread
From: Akinobu Mita @ 2019-01-07 14:20 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: linux-media, open list:OPEN FIRMWARE AND...,
	Guennadi Liakhovetski, Mauro Carvalho Chehab

2019年1月7日(月) 20:37 Sakari Ailus <sakari.ailus@linux.intel.com>:
>
> Hi Mita-san,
>
> On Sun, Dec 23, 2018 at 02:12:45AM +0900, Akinobu Mita wrote:
> > Replace GPL license statements with SPDX license identifiers (GPL-2.0).
> >
> > 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 | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
> > index a1a85ff..65ff59d 100644
> > --- a/drivers/media/i2c/mt9m001.c
> > +++ b/drivers/media/i2c/mt9m001.c
> > @@ -1,11 +1,8 @@
> > +// SPDX-License-Identifier: GPL-2.0
> >  /*
> >   * Driver for MT9M001 CMOS Image Sensor from Micron
> >   *
> >   * Copyright (C) 2008, Guennadi Liakhovetski <kernel@pengutronix.de>
> > - *
> > - * This program is free software; you can redistribute it and/or modify
> > - * it under the terms of the GNU General Public License version 2 as
> > - * published by the Free Software Foundation.
> >   */
> >
> >  #include <linux/videodev2.h>
>
> The MODULE_LICENSE macro at the end of the file lists "GPL" as the license,
> i.e. including later versions. I'm not sure what was the intention
> originally. I guess it's safer to stick to 2.0, at least unless the
> original author is able to shed some light into this.

I've come across the same thought, and I found the following conversation.

https://lkml.org/lkml/2018/6/24/457

So I think MODULE_LICENSE() mismatch can be resolved in the future.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 03/12] media: mt9m001: convert to SPDX license identifer
  2019-01-07 14:20     ` Akinobu Mita
@ 2019-01-07 15:15       ` Sakari Ailus
  0 siblings, 0 replies; 31+ messages in thread
From: Sakari Ailus @ 2019-01-07 15:15 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-media, open list:OPEN FIRMWARE AND...,
	Guennadi Liakhovetski, Mauro Carvalho Chehab

On Mon, Jan 07, 2019 at 11:20:29PM +0900, Akinobu Mita wrote:
> 2019年1月7日(月) 20:37 Sakari Ailus <sakari.ailus@linux.intel.com>:
> >
> > Hi Mita-san,
> >
> > On Sun, Dec 23, 2018 at 02:12:45AM +0900, Akinobu Mita wrote:
> > > Replace GPL license statements with SPDX license identifiers (GPL-2.0).
> > >
> > > 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 | 5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
> > > index a1a85ff..65ff59d 100644
> > > --- a/drivers/media/i2c/mt9m001.c
> > > +++ b/drivers/media/i2c/mt9m001.c
> > > @@ -1,11 +1,8 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > >  /*
> > >   * Driver for MT9M001 CMOS Image Sensor from Micron
> > >   *
> > >   * Copyright (C) 2008, Guennadi Liakhovetski <kernel@pengutronix.de>
> > > - *
> > > - * This program is free software; you can redistribute it and/or modify
> > > - * it under the terms of the GNU General Public License version 2 as
> > > - * published by the Free Software Foundation.
> > >   */
> > >
> > >  #include <linux/videodev2.h>
> >
> > The MODULE_LICENSE macro at the end of the file lists "GPL" as the license,
> > i.e. including later versions. I'm not sure what was the intention
> > originally. I guess it's safer to stick to 2.0, at least unless the
> > original author is able to shed some light into this.
> 
> I've come across the same thought, and I found the following conversation.
> 
> https://lkml.org/lkml/2018/6/24/457
> 
> So I think MODULE_LICENSE() mismatch can be resolved in the future.

I don't see a reason to postpone it. Strictly speaking, for GPL 2+ we'd
need an approval from anyone who has submitted patches to the file. For GPL
v2 it'd be just a patch that fixes the MODULE_LICENSE.

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 12/12] media: mt9m001: set all mbus format field when G_FMT and S_FMT ioctls
  2019-01-07 14:18     ` Akinobu Mita
@ 2019-01-07 15:16       ` Sakari Ailus
  0 siblings, 0 replies; 31+ messages in thread
From: Sakari Ailus @ 2019-01-07 15:16 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-media, open list:OPEN FIRMWARE AND...,
	Guennadi Liakhovetski, Mauro Carvalho Chehab

On Mon, Jan 07, 2019 at 11:18:47PM +0900, Akinobu Mita wrote:
> 2019年1月7日(月) 20:32 Sakari Ailus <sakari.ailus@linux.intel.com>:
> >
> > Hi Mita-san,
> >
> > On Sun, Dec 23, 2018 at 02:12:54AM +0900, Akinobu Mita wrote:
> > > This driver doesn't set all members of mbus format field when the
> > > VIDIOC_SUBDEV_{S,G}_FMT ioctls are called.
> > >
> > > This is detected by v4l2-compliance.
> > >
> > > 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 | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
> > > index f4afbc9..82b89d5 100644
> > > --- a/drivers/media/i2c/mt9m001.c
> > > +++ b/drivers/media/i2c/mt9m001.c
> > > @@ -342,6 +342,9 @@ static int mt9m001_get_fmt(struct v4l2_subdev *sd,
> > >       mf->code        = mt9m001->fmt->code;
> > >       mf->colorspace  = mt9m001->fmt->colorspace;
> > >       mf->field       = V4L2_FIELD_NONE;
> > > +     mf->ycbcr_enc   = V4L2_YCBCR_ENC_DEFAULT;
> > > +     mf->quantization = V4L2_QUANTIZATION_DEFAULT;
> > > +     mf->xfer_func   = V4L2_XFER_FUNC_DEFAULT;
> >
> > Instead of setting the fields individually, would it be feasible to just
> > assign mt9m001->fmt to mf?
> 
> The data type of mt9m001->fmt is struct mt9m001_datafmt, so it can't
> simply assign to mf.

Oh, good point. There are still three places in the driver where you're
setting effectively the same fields; if there is a way to reasonably
address that, it'd be nice to do that. Up to you.

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2019-01-07 15:17 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-22 17:12 [PATCH 00/12] media: mt9m001: switch soc_mt9m001 to a standard subdev sensor driver Akinobu Mita
2018-12-22 17:12 ` [PATCH 01/12] media: i2c: mt9m001: copy mt9m001 soc_camera " Akinobu Mita
2018-12-22 17:12 ` [PATCH 02/12] media: i2c: mt9m001: dt: add binding for mt9m001 Akinobu Mita
2019-01-03 21:12   ` Rob Herring
2019-01-05 15:09     ` Akinobu Mita
2018-12-22 17:12 ` [PATCH 03/12] media: mt9m001: convert to SPDX license identifer Akinobu Mita
2019-01-07 11:37   ` Sakari Ailus
2019-01-07 14:20     ` Akinobu Mita
2019-01-07 15:15       ` Sakari Ailus
2018-12-22 17:12 ` [PATCH 04/12] media: mt9m001: add of_match_table Akinobu Mita
2018-12-22 17:12 ` [PATCH 05/12] media: mt9m001: introduce multi_reg_write() Akinobu Mita
2018-12-22 17:12 ` [PATCH 06/12] media: mt9m001: switch s_power callback to runtime PM Akinobu Mita
     [not found]   ` <20190107100034.po3jsnc3rdj37l4x@kekkonen.localdomain>
2019-01-07 14:07     ` Akinobu Mita
2019-01-07 14:10       ` Sakari Ailus
2018-12-22 17:12 ` [PATCH 07/12] media: mt9m001: remove remaining soc_camera specific code Akinobu Mita
2019-01-07 11:29   ` Sakari Ailus
2019-01-07 14:13     ` Akinobu Mita
2018-12-22 17:12 ` [PATCH 08/12] media: mt9m001: add media controller support Akinobu Mita
2018-12-22 17:12 ` [PATCH 09/12] media: mt9m001: register to V4L2 asynchronous subdevice framework Akinobu Mita
2019-01-07 11:27   ` Sakari Ailus
2019-01-07 14:09     ` Akinobu Mita
2018-12-22 17:12 ` [PATCH 10/12] media: mt9m001: support log_status ioctl and event interface Akinobu Mita
2018-12-22 17:12 ` [PATCH 11/12] media: mt9m001: make VIDIOC_SUBDEV_G_FMT ioctl work with V4L2_SUBDEV_FORMAT_TRY Akinobu Mita
2019-01-07 11:30   ` Sakari Ailus
2019-01-07 14:15     ` Akinobu Mita
2018-12-22 17:12 ` [PATCH 12/12] media: mt9m001: set all mbus format field when G_FMT and S_FMT ioctls Akinobu Mita
2019-01-07 11:32   ` Sakari Ailus
2019-01-07 14:18     ` Akinobu Mita
2019-01-07 15:16       ` Sakari Ailus
2019-01-07  9:36 ` [PATCH 00/12] media: mt9m001: switch soc_mt9m001 to a standard subdev sensor driver Sakari Ailus
2019-01-07  9:48   ` Sakari Ailus

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).