[PATCH 1/4] media: mt9m111: add V4L2_CID_PIXEL_RATE support

[PATCH 1/4] media: mt9m111: add V4L2_CID_PIXEL_RATE support

[Marco Felsch]

Add support to report the PIXEL_RATE so a host or bridge device can ask
the sensor.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/media/i2c/mt9m111.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index afc86efa9e3e..cdaf283e1309 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -908,6 +908,8 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
 		return mt9m111_set_test_pattern(mt9m111, ctrl->val);
 	case V4L2_CID_COLORFX:
 		return mt9m111_set_colorfx(mt9m111, ctrl->val);
+	case V4L2_CID_PIXEL_RATE:
+		return 0;
 	}
 
 	return -EINVAL;
@@ -1249,6 +1251,7 @@ static int mt9m111_probe(struct i2c_client *client)
 {
 	struct mt9m111 *mt9m111;
 	struct i2c_adapter *adapter = client->adapter;
+	unsigned long mclk_rate;
 	int ret;
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
@@ -1271,6 +1274,13 @@ static int mt9m111_probe(struct i2c_client *client)
 	if (IS_ERR(mt9m111->clk))
 		return PTR_ERR(mt9m111->clk);
 
+	ret = clk_prepare_enable(mt9m111->clk);
+	if (ret < 0)
+		return ret;
+
+	mclk_rate = clk_get_rate(mt9m111->clk);
+	clk_disable_unprepare(mt9m111->clk);
+
 	mt9m111->regulator = devm_regulator_get(&client->dev, "vdd");
 	if (IS_ERR(mt9m111->regulator)) {
 		dev_err(&client->dev, "regulator not found: %ld\n",
@@ -1285,7 +1295,7 @@ static int mt9m111_probe(struct i2c_client *client)
 	mt9m111->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
 				 V4L2_SUBDEV_FL_HAS_EVENTS;
 
-	v4l2_ctrl_handler_init(&mt9m111->hdl, 7);
+	v4l2_ctrl_handler_init(&mt9m111->hdl, 8);
 	v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
 			V4L2_CID_VFLIP, 0, 1, 1, 0);
 	v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
@@ -1309,6 +1319,9 @@ static int mt9m111_probe(struct i2c_client *client)
 				BIT(V4L2_COLORFX_NEGATIVE) |
 				BIT(V4L2_COLORFX_SOLARIZATION)),
 			V4L2_COLORFX_NONE);
+	v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops, V4L2_CID_PIXEL_RATE,
+			  mclk_rate, mclk_rate, 1, mclk_rate);
+
 	mt9m111->subdev.ctrl_handler = &mt9m111->hdl;
 	if (mt9m111->hdl.error) {
 		ret = mt9m111->hdl.error;
-- 
2.30.2

[PATCH 2/4] media: mt9m111: fix subdev API usage

[Marco Felsch]

In case of I2C transfer failures the driver return failure codes which
are not allowed according the API documentation. Fix that by ignore the
register access failure codes.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/media/i2c/mt9m111.c | 116 ++++++++++++++++++++----------------
 1 file changed, 66 insertions(+), 50 deletions(-)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index cdaf283e1309..53c4dac4e4bd 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -455,7 +455,7 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd,
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
 	struct v4l2_rect rect = sel->r;
 	int width, height;
-	int ret, align = 0;
+	int align = 0;
 
 	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE ||
 	    sel->target != V4L2_SEL_TGT_CROP)
@@ -481,14 +481,13 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd,
 	width = min(mt9m111->width, rect.width);
 	height = min(mt9m111->height, rect.height);
 
-	ret = mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code);
-	if (!ret) {
-		mt9m111->rect = rect;
-		mt9m111->width = width;
-		mt9m111->height = height;
-	}
 
-	return ret;
+	mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code);
+	mt9m111->rect = rect;
+	mt9m111->width = width;
+	mt9m111->height = height;
+
+	return 0;
 }
 
 static int mt9m111_get_selection(struct v4l2_subdev *sd,
@@ -558,7 +557,6 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
 		MT9M111_OUTFMT_RGB444x | MT9M111_OUTFMT_RGBx444 |
 		MT9M111_OUTFMT_SWAP_YCbCr_C_Y_RGB_EVEN |
 		MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr_RGB_R_B;
-	int ret;
 
 	switch (code) {
 	case MEDIA_BUS_FMT_SBGGR8_1X8:
@@ -613,13 +611,13 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
 	if (mt9m111->pclk_sample == 0)
 		mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK;
 
-	ret = mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
-			       data_outfmt2, mask_outfmt2);
-	if (!ret)
-		ret = mt9m111_reg_mask(client, context_b.output_fmt_ctrl2,
-				       data_outfmt2, mask_outfmt2);
 
-	return ret;
+	mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
+			 data_outfmt2, mask_outfmt2);
+	mt9m111_reg_mask(client, context_b.output_fmt_ctrl2,
+			 data_outfmt2, mask_outfmt2);
+
+	return 0;
 }
 
 static int mt9m111_set_fmt(struct v4l2_subdev *sd,
@@ -632,7 +630,6 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
 	const struct mt9m111_datafmt *fmt;
 	struct v4l2_rect *rect = &mt9m111->rect;
 	bool bayer;
-	int ret;
 
 	if (mt9m111->is_streaming)
 		return -EBUSY;
@@ -681,16 +678,14 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
 		return 0;
 	}
 
-	ret = mt9m111_setup_geometry(mt9m111, rect, mf->width, mf->height, mf->code);
-	if (!ret)
-		ret = mt9m111_set_pixfmt(mt9m111, mf->code);
-	if (!ret) {
-		mt9m111->width	= mf->width;
-		mt9m111->height	= mf->height;
-		mt9m111->fmt	= fmt;
-	}
 
-	return ret;
+	mt9m111_setup_geometry(mt9m111, rect, mf->width, mf->height, mf->code);
+	mt9m111_set_pixfmt(mt9m111, mf->code);
+	mt9m111->width	= mf->width;
+	mt9m111->height	= mf->height;
+	mt9m111->fmt	= fmt;
+
+	return 0;
 }
 
 static const struct mt9m111_mode_info *
@@ -786,14 +781,13 @@ static int mt9m111_s_register(struct v4l2_subdev *sd,
 static int mt9m111_set_flip(struct mt9m111 *mt9m111, int flip, int mask)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
-	int ret;
 
 	if (flip)
-		ret = mt9m111_reg_set(client, mt9m111->ctx->read_mode, mask);
+		mt9m111_reg_set(client, mt9m111->ctx->read_mode, mask);
 	else
-		ret = mt9m111_reg_clear(client, mt9m111->ctx->read_mode, mask);
+		mt9m111_reg_clear(client, mt9m111->ctx->read_mode, mask);
 
-	return ret;
+	return 0;
 }
 
 static int mt9m111_get_global_gain(struct mt9m111 *mt9m111)
@@ -823,7 +817,9 @@ static int mt9m111_set_global_gain(struct mt9m111 *mt9m111, int gain)
 	else
 		val = gain;
 
-	return reg_write(GLOBAL_GAIN, val);
+	reg_write(GLOBAL_GAIN, val);
+
+	return 0;
 }
 
 static int mt9m111_set_autoexposure(struct mt9m111 *mt9m111, int val)
@@ -831,8 +827,11 @@ static int mt9m111_set_autoexposure(struct mt9m111 *mt9m111, int val)
 	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
 
 	if (val == V4L2_EXPOSURE_AUTO)
-		return reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
-	return reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
+		reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
+	else
+		reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
+
+	return 0;
 }
 
 static int mt9m111_set_autowhitebalance(struct mt9m111 *mt9m111, int on)
@@ -840,8 +839,11 @@ static int mt9m111_set_autowhitebalance(struct mt9m111 *mt9m111, int on)
 	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
 
 	if (on)
-		return reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
-	return reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
+		reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
+	else
+		reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
+
+	return 0;
 }
 
 static const char * const mt9m111_test_pattern_menu[] = {
@@ -859,8 +861,9 @@ static int mt9m111_set_test_pattern(struct mt9m111 *mt9m111, int val)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
 
-	return mt9m111_reg_mask(client, MT9M111_TPG_CTRL, val,
-				MT9M111_TPG_SEL_MASK);
+	mt9m111_reg_mask(client, MT9M111_TPG_CTRL, val, MT9M111_TPG_SEL_MASK);
+
+	return 0;
 }
 
 static int mt9m111_set_colorfx(struct mt9m111 *mt9m111, int val)
@@ -877,9 +880,10 @@ static int mt9m111_set_colorfx(struct mt9m111 *mt9m111, int val)
 
 	for (i = 0; i < ARRAY_SIZE(colorfx); i++) {
 		if (colorfx[i].id == val) {
-			return mt9m111_reg_mask(client, MT9M111_EFFECTS_MODE,
-						colorfx[i].value,
-						MT9M111_EFFECTS_MODE_MASK);
+			mt9m111_reg_mask(client, MT9M111_EFFECTS_MODE,
+					 colorfx[i].value,
+					 MT9M111_EFFECTS_MODE_MASK);
+			return 0;
 		}
 	}
 
@@ -890,29 +894,41 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct mt9m111 *mt9m111 = container_of(ctrl->handler,
 					       struct mt9m111, hdl);
+	int ret;
 
 	switch (ctrl->id) {
 	case V4L2_CID_VFLIP:
-		return mt9m111_set_flip(mt9m111, ctrl->val,
-					MT9M111_RMB_MIRROR_ROWS);
+		ret = mt9m111_set_flip(mt9m111, ctrl->val,
+				       MT9M111_RMB_MIRROR_ROWS);
+		break;
 	case V4L2_CID_HFLIP:
-		return mt9m111_set_flip(mt9m111, ctrl->val,
-					MT9M111_RMB_MIRROR_COLS);
+		ret = mt9m111_set_flip(mt9m111, ctrl->val,
+				       MT9M111_RMB_MIRROR_COLS);
+		break;
 	case V4L2_CID_GAIN:
-		return mt9m111_set_global_gain(mt9m111, ctrl->val);
+		ret = mt9m111_set_global_gain(mt9m111, ctrl->val);
+		break;
 	case V4L2_CID_EXPOSURE_AUTO:
-		return mt9m111_set_autoexposure(mt9m111, ctrl->val);
+		ret = mt9m111_set_autoexposure(mt9m111, ctrl->val);
+		break;
 	case V4L2_CID_AUTO_WHITE_BALANCE:
-		return mt9m111_set_autowhitebalance(mt9m111, ctrl->val);
+		ret = mt9m111_set_autowhitebalance(mt9m111, ctrl->val);
+		break;
 	case V4L2_CID_TEST_PATTERN:
-		return mt9m111_set_test_pattern(mt9m111, ctrl->val);
+		ret = mt9m111_set_test_pattern(mt9m111, ctrl->val);
+		break;
 	case V4L2_CID_COLORFX:
-		return mt9m111_set_colorfx(mt9m111, ctrl->val);
+		ret = mt9m111_set_colorfx(mt9m111, ctrl->val);
+		break;
 	case V4L2_CID_PIXEL_RATE:
-		return 0;
+		ret = 0;
+		break;
+	default:
+		ret = -EINVAL;
 	}
 
-	return -EINVAL;
+
+	return ret;
 }
 
 static int mt9m111_suspend(struct mt9m111 *mt9m111)
-- 
2.30.2

[PATCH 3/4] media: mt9m111: fix device power usage

[Marco Felsch]

Currently the driver turn off the power after probe and toggle it during
.stream by using the .s_power callback. This is problematic since other
callbacks like .set_fmt accessing the hardware as well which will fail.
So in the end the default format is the only supported format.

Remove the hardware register access from the callbacks and instead sync
the state once right before the stream gets enabled to fix this.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/media/i2c/mt9m111.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 53c4dac4e4bd..cd74c408e110 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -481,8 +481,6 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd,
 	width = min(mt9m111->width, rect.width);
 	height = min(mt9m111->height, rect.height);
 
-
-	mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code);
 	mt9m111->rect = rect;
 	mt9m111->width = width;
 	mt9m111->height = height;
@@ -611,7 +609,6 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
 	if (mt9m111->pclk_sample == 0)
 		mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK;
 
-
 	mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
 			 data_outfmt2, mask_outfmt2);
 	mt9m111_reg_mask(client, context_b.output_fmt_ctrl2,
@@ -678,9 +675,6 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
 		return 0;
 	}
 
-
-	mt9m111_setup_geometry(mt9m111, rect, mf->width, mf->height, mf->code);
-	mt9m111_set_pixfmt(mt9m111, mf->code);
 	mt9m111->width	= mf->width;
 	mt9m111->height	= mf->height;
 	mt9m111->fmt	= fmt;
@@ -743,6 +737,8 @@ mt9m111_find_mode(struct mt9m111 *mt9m111, unsigned int req_fps,
 	return mode;
 }
 
+static int mt9m111_s_power(struct v4l2_subdev *sd, int on);
+
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 static int mt9m111_g_register(struct v4l2_subdev *sd,
 			      struct v4l2_dbg_register *reg)
@@ -753,10 +749,14 @@ static int mt9m111_g_register(struct v4l2_subdev *sd,
 	if (reg->reg > 0x2ff)
 		return -EINVAL;
 
+	mt9m111_s_power(sd, 1);
+
 	val = mt9m111_reg_read(client, reg->reg);
 	reg->size = 2;
 	reg->val = (u64)val;
 
+	mt9m111_s_power(sd, 0);
+
 	if (reg->val > 0xffff)
 		return -EIO;
 
@@ -771,9 +771,13 @@ static int mt9m111_s_register(struct v4l2_subdev *sd,
 	if (reg->reg > 0x2ff)
 		return -EINVAL;
 
+	mt9m111_s_power(sd, 1);
+
 	if (mt9m111_reg_write(client, reg->reg, reg->val) < 0)
 		return -EIO;
 
+	mt9m111_s_power(sd, 0);
+
 	return 0;
 }
 #endif
@@ -896,6 +900,9 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
 					       struct mt9m111, hdl);
 	int ret;
 
+	if (!mt9m111->is_streaming)
+		return 0;
+
 	switch (ctrl->id) {
 	case V4L2_CID_VFLIP:
 		ret = mt9m111_set_flip(mt9m111, ctrl->val,
@@ -927,7 +934,6 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
 		ret = -EINVAL;
 	}
 
-
 	return ret;
 }
 
-- 
2.30.2

[PATCH 4/4] media: mt9m111: remove .s_power callback

[Marco Felsch]

This is in preparation of switching to the generic dev PM mechanism.
Since the .s_power callback will be removed in the near future move the
powering into the .s_stream callback. So this driver no longer depends
on the .s_power mechanism.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/media/i2c/mt9m111.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index cd74c408e110..8e8ba5a8e6ea 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -1065,7 +1065,6 @@ static const struct v4l2_ctrl_ops mt9m111_ctrl_ops = {
 };
 
 static const struct v4l2_subdev_core_ops mt9m111_subdev_core_ops = {
-	.s_power	= mt9m111_s_power,
 	.log_status = v4l2_ctrl_subdev_log_status,
 	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
 	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
@@ -1136,8 +1135,14 @@ static int mt9m111_enum_mbus_code(struct v4l2_subdev *sd,
 static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
+	int ret;
 
 	mt9m111->is_streaming = !!enable;
+
+	ret = mt9m111_s_power(sd, enable);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
-- 
2.30.2

Re: [PATCH 1/4] media: mt9m111: add V4L2_CID_PIXEL_RATE support

[Jacopo Mondi]

Hi Marco

On Thu, Aug 18, 2022 at 04:47:09PM +0200, Marco Felsch wrote:
> Add support to report the PIXEL_RATE so a host or bridge device can ask
> the sensor.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/media/i2c/mt9m111.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index afc86efa9e3e..cdaf283e1309 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -908,6 +908,8 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
>  		return mt9m111_set_test_pattern(mt9m111, ctrl->val);
>  	case V4L2_CID_COLORFX:
>  		return mt9m111_set_colorfx(mt9m111, ctrl->val);
> +	case V4L2_CID_PIXEL_RATE:
> +		return 0;

By default PIXEL_RATE is read-only.
Do you get a call to s_ctrl for it ?

>  	}
>
>  	return -EINVAL;
> @@ -1249,6 +1251,7 @@ static int mt9m111_probe(struct i2c_client *client)
>  {
>  	struct mt9m111 *mt9m111;
>  	struct i2c_adapter *adapter = client->adapter;
> +	unsigned long mclk_rate;
>  	int ret;
>
>  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
> @@ -1271,6 +1274,13 @@ static int mt9m111_probe(struct i2c_client *client)
>  	if (IS_ERR(mt9m111->clk))
>  		return PTR_ERR(mt9m111->clk);
>
> +	ret = clk_prepare_enable(mt9m111->clk);
> +	if (ret < 0)
> +		return ret;
> +

Do you need to enable clock to read its rate ?

> +	mclk_rate = clk_get_rate(mt9m111->clk);
> +	clk_disable_unprepare(mt9m111->clk);
> +
>  	mt9m111->regulator = devm_regulator_get(&client->dev, "vdd");
>  	if (IS_ERR(mt9m111->regulator)) {
>  		dev_err(&client->dev, "regulator not found: %ld\n",
> @@ -1285,7 +1295,7 @@ static int mt9m111_probe(struct i2c_client *client)
>  	mt9m111->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
>  				 V4L2_SUBDEV_FL_HAS_EVENTS;
>
> -	v4l2_ctrl_handler_init(&mt9m111->hdl, 7);
> +	v4l2_ctrl_handler_init(&mt9m111->hdl, 8);
>  	v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
>  			V4L2_CID_VFLIP, 0, 1, 1, 0);
>  	v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> @@ -1309,6 +1319,9 @@ static int mt9m111_probe(struct i2c_client *client)
>  				BIT(V4L2_COLORFX_NEGATIVE) |
>  				BIT(V4L2_COLORFX_SOLARIZATION)),
>  			V4L2_COLORFX_NONE);
> +	v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops, V4L2_CID_PIXEL_RATE,
> +			  mclk_rate, mclk_rate, 1, mclk_rate);
> +

I don't have a datasheet but it seems a little weird that the mclk
frequency is the same as the pixel clock rate ?

>  	mt9m111->subdev.ctrl_handler = &mt9m111->hdl;
>  	if (mt9m111->hdl.error) {
>  		ret = mt9m111->hdl.error;
> --
> 2.30.2
>

Re: [PATCH 4/4] media: mt9m111: remove .s_power callback

[Jacopo Mondi]

Hi Marco

On Thu, Aug 18, 2022 at 04:47:12PM +0200, Marco Felsch wrote:
> This is in preparation of switching to the generic dev PM mechanism.
> Since the .s_power callback will be removed in the near future move the
> powering into the .s_stream callback. So this driver no longer depends
> on the .s_power mechanism.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

If you want to move to runtime_pm, I would implement it first and have
s_power call the _resume and _suspend routines, as some platform
drivers still use s_power() and some of them might depend on it.

It's a slippery slope.. I would love to get rid of s_power() but if
any platform uses it with this sensor, it would stop working after
this change.

> ---
>  drivers/media/i2c/mt9m111.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index cd74c408e110..8e8ba5a8e6ea 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -1065,7 +1065,6 @@ static const struct v4l2_ctrl_ops mt9m111_ctrl_ops = {
>  };
>
>  static const struct v4l2_subdev_core_ops mt9m111_subdev_core_ops = {
> -	.s_power	= mt9m111_s_power,
>  	.log_status = v4l2_ctrl_subdev_log_status,
>  	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
>  	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
> @@ -1136,8 +1135,14 @@ static int mt9m111_enum_mbus_code(struct v4l2_subdev *sd,
>  static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
> +	int ret;
>
>  	mt9m111->is_streaming = !!enable;
> +
> +	ret = mt9m111_s_power(sd, enable);
> +	if (ret)
> +		return ret;
> +
>  	return 0;
>  }
>
> --
> 2.30.2
>

Re: [PATCH 2/4] media: mt9m111: fix subdev API usage

[Jacopo Mondi]

Hi Marco

On Thu, Aug 18, 2022 at 04:47:10PM +0200, Marco Felsch wrote:
> In case of I2C transfer failures the driver return failure codes which
> are not allowed according the API documentation. Fix that by ignore the
> register access failure codes.

I might have missed the reason why subdev ops are not allowed to
fail..

Also you're here changing both subdev ops handler and function that handle
controls if I'm not mistaken.

>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/media/i2c/mt9m111.c | 116 ++++++++++++++++++++----------------
>  1 file changed, 66 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index cdaf283e1309..53c4dac4e4bd 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -455,7 +455,7 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd,
>  	struct mt9m111 *mt9m111 = to_mt9m111(client);
>  	struct v4l2_rect rect = sel->r;
>  	int width, height;
> -	int ret, align = 0;
> +	int align = 0;
>
>  	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE ||
>  	    sel->target != V4L2_SEL_TGT_CROP)
> @@ -481,14 +481,13 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd,
>  	width = min(mt9m111->width, rect.width);
>  	height = min(mt9m111->height, rect.height);
>
> -	ret = mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code);
> -	if (!ret) {
> -		mt9m111->rect = rect;
> -		mt9m111->width = width;
> -		mt9m111->height = height;
> -	}
>
> -	return ret;
> +	mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code);
> +	mt9m111->rect = rect;
> +	mt9m111->width = width;
> +	mt9m111->height = height;
> +
> +	return 0;
>  }
>
>  static int mt9m111_get_selection(struct v4l2_subdev *sd,
> @@ -558,7 +557,6 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
>  		MT9M111_OUTFMT_RGB444x | MT9M111_OUTFMT_RGBx444 |
>  		MT9M111_OUTFMT_SWAP_YCbCr_C_Y_RGB_EVEN |
>  		MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr_RGB_R_B;
> -	int ret;
>
>  	switch (code) {
>  	case MEDIA_BUS_FMT_SBGGR8_1X8:
> @@ -613,13 +611,13 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
>  	if (mt9m111->pclk_sample == 0)
>  		mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK;
>
> -	ret = mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
> -			       data_outfmt2, mask_outfmt2);
> -	if (!ret)
> -		ret = mt9m111_reg_mask(client, context_b.output_fmt_ctrl2,
> -				       data_outfmt2, mask_outfmt2);
>
> -	return ret;
> +	mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
> +			 data_outfmt2, mask_outfmt2);
> +	mt9m111_reg_mask(client, context_b.output_fmt_ctrl2,
> +			 data_outfmt2, mask_outfmt2);
> +
> +	return 0;
>  }
>
>  static int mt9m111_set_fmt(struct v4l2_subdev *sd,
> @@ -632,7 +630,6 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
>  	const struct mt9m111_datafmt *fmt;
>  	struct v4l2_rect *rect = &mt9m111->rect;
>  	bool bayer;
> -	int ret;
>
>  	if (mt9m111->is_streaming)
>  		return -EBUSY;
> @@ -681,16 +678,14 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
>  		return 0;
>  	}
>
> -	ret = mt9m111_setup_geometry(mt9m111, rect, mf->width, mf->height, mf->code);
> -	if (!ret)
> -		ret = mt9m111_set_pixfmt(mt9m111, mf->code);
> -	if (!ret) {
> -		mt9m111->width	= mf->width;
> -		mt9m111->height	= mf->height;
> -		mt9m111->fmt	= fmt;
> -	}
>
> -	return ret;
> +	mt9m111_setup_geometry(mt9m111, rect, mf->width, mf->height, mf->code);
> +	mt9m111_set_pixfmt(mt9m111, mf->code);
> +	mt9m111->width	= mf->width;
> +	mt9m111->height	= mf->height;
> +	mt9m111->fmt	= fmt;
> +
> +	return 0;
>  }
>
>  static const struct mt9m111_mode_info *
> @@ -786,14 +781,13 @@ static int mt9m111_s_register(struct v4l2_subdev *sd,
>  static int mt9m111_set_flip(struct mt9m111 *mt9m111, int flip, int mask)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
> -	int ret;
>
>  	if (flip)
> -		ret = mt9m111_reg_set(client, mt9m111->ctx->read_mode, mask);
> +		mt9m111_reg_set(client, mt9m111->ctx->read_mode, mask);
>  	else
> -		ret = mt9m111_reg_clear(client, mt9m111->ctx->read_mode, mask);
> +		mt9m111_reg_clear(client, mt9m111->ctx->read_mode, mask);
>
> -	return ret;
> +	return 0;
>  }
>
>  static int mt9m111_get_global_gain(struct mt9m111 *mt9m111)
> @@ -823,7 +817,9 @@ static int mt9m111_set_global_gain(struct mt9m111 *mt9m111, int gain)
>  	else
>  		val = gain;
>
> -	return reg_write(GLOBAL_GAIN, val);
> +	reg_write(GLOBAL_GAIN, val);
> +
> +	return 0;
>  }
>
>  static int mt9m111_set_autoexposure(struct mt9m111 *mt9m111, int val)
> @@ -831,8 +827,11 @@ static int mt9m111_set_autoexposure(struct mt9m111 *mt9m111, int val)
>  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
>
>  	if (val == V4L2_EXPOSURE_AUTO)
> -		return reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
> -	return reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
> +		reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
> +	else
> +		reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
> +
> +	return 0;
>  }
>
>  static int mt9m111_set_autowhitebalance(struct mt9m111 *mt9m111, int on)
> @@ -840,8 +839,11 @@ static int mt9m111_set_autowhitebalance(struct mt9m111 *mt9m111, int on)
>  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
>
>  	if (on)
> -		return reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
> -	return reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
> +		reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
> +	else
> +		reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
> +
> +	return 0;
>  }
>
>  static const char * const mt9m111_test_pattern_menu[] = {
> @@ -859,8 +861,9 @@ static int mt9m111_set_test_pattern(struct mt9m111 *mt9m111, int val)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
>
> -	return mt9m111_reg_mask(client, MT9M111_TPG_CTRL, val,
> -				MT9M111_TPG_SEL_MASK);
> +	mt9m111_reg_mask(client, MT9M111_TPG_CTRL, val, MT9M111_TPG_SEL_MASK);
> +
> +	return 0;
>  }
>
>  static int mt9m111_set_colorfx(struct mt9m111 *mt9m111, int val)
> @@ -877,9 +880,10 @@ static int mt9m111_set_colorfx(struct mt9m111 *mt9m111, int val)
>
>  	for (i = 0; i < ARRAY_SIZE(colorfx); i++) {
>  		if (colorfx[i].id == val) {
> -			return mt9m111_reg_mask(client, MT9M111_EFFECTS_MODE,
> -						colorfx[i].value,
> -						MT9M111_EFFECTS_MODE_MASK);
> +			mt9m111_reg_mask(client, MT9M111_EFFECTS_MODE,
> +					 colorfx[i].value,
> +					 MT9M111_EFFECTS_MODE_MASK);
> +			return 0;
>  		}
>  	}
>
> @@ -890,29 +894,41 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct mt9m111 *mt9m111 = container_of(ctrl->handler,
>  					       struct mt9m111, hdl);
> +	int ret;
>
>  	switch (ctrl->id) {
>  	case V4L2_CID_VFLIP:
> -		return mt9m111_set_flip(mt9m111, ctrl->val,
> -					MT9M111_RMB_MIRROR_ROWS);
> +		ret = mt9m111_set_flip(mt9m111, ctrl->val,
> +				       MT9M111_RMB_MIRROR_ROWS);
> +		break;
>  	case V4L2_CID_HFLIP:
> -		return mt9m111_set_flip(mt9m111, ctrl->val,
> -					MT9M111_RMB_MIRROR_COLS);
> +		ret = mt9m111_set_flip(mt9m111, ctrl->val,
> +				       MT9M111_RMB_MIRROR_COLS);
> +		break;
>  	case V4L2_CID_GAIN:
> -		return mt9m111_set_global_gain(mt9m111, ctrl->val);
> +		ret = mt9m111_set_global_gain(mt9m111, ctrl->val);
> +		break;
>  	case V4L2_CID_EXPOSURE_AUTO:
> -		return mt9m111_set_autoexposure(mt9m111, ctrl->val);
> +		ret = mt9m111_set_autoexposure(mt9m111, ctrl->val);
> +		break;
>  	case V4L2_CID_AUTO_WHITE_BALANCE:
> -		return mt9m111_set_autowhitebalance(mt9m111, ctrl->val);
> +		ret = mt9m111_set_autowhitebalance(mt9m111, ctrl->val);
> +		break;
>  	case V4L2_CID_TEST_PATTERN:
> -		return mt9m111_set_test_pattern(mt9m111, ctrl->val);
> +		ret = mt9m111_set_test_pattern(mt9m111, ctrl->val);
> +		break;
>  	case V4L2_CID_COLORFX:
> -		return mt9m111_set_colorfx(mt9m111, ctrl->val);
> +		ret = mt9m111_set_colorfx(mt9m111, ctrl->val);
> +		break;
>  	case V4L2_CID_PIXEL_RATE:
> -		return 0;
> +		ret = 0;
> +		break;
> +	default:
> +		ret = -EINVAL;
>  	}
>
> -	return -EINVAL;
> +
> +	return ret;
>  }
>
>  static int mt9m111_suspend(struct mt9m111 *mt9m111)
> --
> 2.30.2
>

Re: [PATCH 4/4] media: mt9m111: remove .s_power callback

[Marco Felsch]

Hi Jacopo,

thanks for your fast feedback :)

On 22-08-18, Jacopo Mondi wrote:
> Hi Marco
> 
> On Thu, Aug 18, 2022 at 04:47:12PM +0200, Marco Felsch wrote:
> > This is in preparation of switching to the generic dev PM mechanism.
> > Since the .s_power callback will be removed in the near future move the
> > powering into the .s_stream callback. So this driver no longer depends
> > on the .s_power mechanism.
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> 
> If you want to move to runtime_pm, I would implement it first and have
> s_power call the _resume and _suspend routines, as some platform
> drivers still use s_power() and some of them might depend on it.

Do we really have platforms which depend on this? IMHO if that is the
case than we should fix those platfoms. Since new drivers shouldn't use
this callback anymore.

In my case, I worked on [1] and wondered why the sensor was enabled
before I told him to do so. Since I didn't implement the s_power()
callback, I had no chance to get enabled before.

Can we please decide:
 - Do we wanna get rid of the s_power() callback?
 - If not, how do we handle those devices then with drivers not
   implementing this callback?

[1] https://lore.kernel.org/all/20220818143307.967150-1-m.felsch@pengutronix.de/

> It's a slippery slope.. I would love to get rid of s_power() but if
> any platform uses it with this sensor, it would stop working after
> this change.
> 
> > ---
> >  drivers/media/i2c/mt9m111.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > index cd74c408e110..8e8ba5a8e6ea 100644
> > --- a/drivers/media/i2c/mt9m111.c
> > +++ b/drivers/media/i2c/mt9m111.c
> > @@ -1065,7 +1065,6 @@ static const struct v4l2_ctrl_ops mt9m111_ctrl_ops = {
> >  };
> >
> >  static const struct v4l2_subdev_core_ops mt9m111_subdev_core_ops = {
> > -	.s_power	= mt9m111_s_power,
> >  	.log_status = v4l2_ctrl_subdev_log_status,
> >  	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> >  	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
> > @@ -1136,8 +1135,14 @@ static int mt9m111_enum_mbus_code(struct v4l2_subdev *sd,
> >  static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable)
> >  {
> >  	struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
> > +	int ret;
> >
> >  	mt9m111->is_streaming = !!enable;
> > +
> > +	ret = mt9m111_s_power(sd, enable);
> > +	if (ret)
> > +		return ret;
> > +
> >  	return 0;
> >  }
> >
> > --
> > 2.30.2
> >
> 

Re: [PATCH 3/4] media: mt9m111: fix device power usage

[Jacopo Mondi]

Hi Marco

On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote:
> Currently the driver turn off the power after probe and toggle it during
> .stream by using the .s_power callback. This is problematic since other
> callbacks like .set_fmt accessing the hardware as well which will fail.

Ouch!

> So in the end the default format is the only supported format.
>
> Remove the hardware register access from the callbacks and instead sync
> the state once right before the stream gets enabled to fix this.

Where does it happen in this patch ?

>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/media/i2c/mt9m111.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index 53c4dac4e4bd..cd74c408e110 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -481,8 +481,6 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd,
>  	width = min(mt9m111->width, rect.width);
>  	height = min(mt9m111->height, rect.height);
>
> -

Why in mainline I don't see these empty lines ?

> -	mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code);
>  	mt9m111->rect = rect;
>  	mt9m111->width = width;
>  	mt9m111->height = height;
> @@ -611,7 +609,6 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
>  	if (mt9m111->pclk_sample == 0)
>  		mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK;
>
> -
>  	mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
>  			 data_outfmt2, mask_outfmt2);
>  	mt9m111_reg_mask(client, context_b.output_fmt_ctrl2,
> @@ -678,9 +675,6 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
>  		return 0;
>  	}
>
> -
> -	mt9m111_setup_geometry(mt9m111, rect, mf->width, mf->height, mf->code);
> -	mt9m111_set_pixfmt(mt9m111, mf->code);

Are we looking at two different versions of the driver ??
https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/mt9m111.c#L684

>  	mt9m111->width	= mf->width;
>  	mt9m111->height	= mf->height;
>  	mt9m111->fmt	= fmt;
> @@ -743,6 +737,8 @@ mt9m111_find_mode(struct mt9m111 *mt9m111, unsigned int req_fps,
>  	return mode;
>  }
>
> +static int mt9m111_s_power(struct v4l2_subdev *sd, int on);
> +
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>  static int mt9m111_g_register(struct v4l2_subdev *sd,
>  			      struct v4l2_dbg_register *reg)
> @@ -753,10 +749,14 @@ static int mt9m111_g_register(struct v4l2_subdev *sd,
>  	if (reg->reg > 0x2ff)
>  		return -EINVAL;
>
> +	mt9m111_s_power(sd, 1);
> +
>  	val = mt9m111_reg_read(client, reg->reg);
>  	reg->size = 2;
>  	reg->val = (u64)val;
>
> +	mt9m111_s_power(sd, 0);
> +
>  	if (reg->val > 0xffff)
>  		return -EIO;
>
> @@ -771,9 +771,13 @@ static int mt9m111_s_register(struct v4l2_subdev *sd,
>  	if (reg->reg > 0x2ff)
>  		return -EINVAL;
>
> +	mt9m111_s_power(sd, 1);
> +
>  	if (mt9m111_reg_write(client, reg->reg, reg->val) < 0)
>  		return -EIO;
>
> +	mt9m111_s_power(sd, 0);
> +
>  	return 0;
>  }
>  #endif
> @@ -896,6 +900,9 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
>  					       struct mt9m111, hdl);
>  	int ret;
>
> +	if (!mt9m111->is_streaming)
> +		return 0;
> +
>  	switch (ctrl->id) {
>  	case V4L2_CID_VFLIP:
>  		ret = mt9m111_set_flip(mt9m111, ctrl->val,
> @@ -927,7 +934,6 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
>  		ret = -EINVAL;
>  	}
>
> -
>  	return ret;
>  }
>
> --
> 2.30.2
>

Re: [PATCH 2/4] media: mt9m111: fix subdev API usage

[Marco Felsch]

Hi Jacopo,

On 22-08-19, Jacopo Mondi wrote:
> Hi Marco
> 
> On Thu, Aug 18, 2022 at 04:47:10PM +0200, Marco Felsch wrote:
> > In case of I2C transfer failures the driver return failure codes which
> > are not allowed according the API documentation. Fix that by ignore the
> > register access failure codes.
> 
> I might have missed the reason why subdev ops are not allowed to
> fail..

Please see the links below:

https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-subdev-g-fmt.html?highlight=subdev_s_fmt#return-value
https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-subdev-g-selection.html#return-value
https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-g-ext-ctrls.html#return-value

There is e.g. no ENXIO error code allowed.

> Also you're here changing both subdev ops handler and function that handle
> controls if I'm not mistaken.

Yes I did that for the controls because the errors are incorrect there
as well. You're right, I forgot to mention this in the commit message.

Regards,
  Marco

> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/media/i2c/mt9m111.c | 116 ++++++++++++++++++++----------------
> >  1 file changed, 66 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > index cdaf283e1309..53c4dac4e4bd 100644
> > --- a/drivers/media/i2c/mt9m111.c
> > +++ b/drivers/media/i2c/mt9m111.c
> > @@ -455,7 +455,7 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd,
> >  	struct mt9m111 *mt9m111 = to_mt9m111(client);
> >  	struct v4l2_rect rect = sel->r;
> >  	int width, height;
> > -	int ret, align = 0;
> > +	int align = 0;
> >
> >  	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE ||
> >  	    sel->target != V4L2_SEL_TGT_CROP)
> > @@ -481,14 +481,13 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd,
> >  	width = min(mt9m111->width, rect.width);
> >  	height = min(mt9m111->height, rect.height);
> >
> > -	ret = mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code);
> > -	if (!ret) {
> > -		mt9m111->rect = rect;
> > -		mt9m111->width = width;
> > -		mt9m111->height = height;
> > -	}
> >
> > -	return ret;
> > +	mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code);
> > +	mt9m111->rect = rect;
> > +	mt9m111->width = width;
> > +	mt9m111->height = height;
> > +
> > +	return 0;
> >  }
> >
> >  static int mt9m111_get_selection(struct v4l2_subdev *sd,
> > @@ -558,7 +557,6 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
> >  		MT9M111_OUTFMT_RGB444x | MT9M111_OUTFMT_RGBx444 |
> >  		MT9M111_OUTFMT_SWAP_YCbCr_C_Y_RGB_EVEN |
> >  		MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr_RGB_R_B;
> > -	int ret;
> >
> >  	switch (code) {
> >  	case MEDIA_BUS_FMT_SBGGR8_1X8:
> > @@ -613,13 +611,13 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
> >  	if (mt9m111->pclk_sample == 0)
> >  		mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK;
> >
> > -	ret = mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
> > -			       data_outfmt2, mask_outfmt2);
> > -	if (!ret)
> > -		ret = mt9m111_reg_mask(client, context_b.output_fmt_ctrl2,
> > -				       data_outfmt2, mask_outfmt2);
> >
> > -	return ret;
> > +	mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
> > +			 data_outfmt2, mask_outfmt2);
> > +	mt9m111_reg_mask(client, context_b.output_fmt_ctrl2,
> > +			 data_outfmt2, mask_outfmt2);
> > +
> > +	return 0;
> >  }
> >
> >  static int mt9m111_set_fmt(struct v4l2_subdev *sd,
> > @@ -632,7 +630,6 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
> >  	const struct mt9m111_datafmt *fmt;
> >  	struct v4l2_rect *rect = &mt9m111->rect;
> >  	bool bayer;
> > -	int ret;
> >
> >  	if (mt9m111->is_streaming)
> >  		return -EBUSY;
> > @@ -681,16 +678,14 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
> >  		return 0;
> >  	}
> >
> > -	ret = mt9m111_setup_geometry(mt9m111, rect, mf->width, mf->height, mf->code);
> > -	if (!ret)
> > -		ret = mt9m111_set_pixfmt(mt9m111, mf->code);
> > -	if (!ret) {
> > -		mt9m111->width	= mf->width;
> > -		mt9m111->height	= mf->height;
> > -		mt9m111->fmt	= fmt;
> > -	}
> >
> > -	return ret;
> > +	mt9m111_setup_geometry(mt9m111, rect, mf->width, mf->height, mf->code);
> > +	mt9m111_set_pixfmt(mt9m111, mf->code);
> > +	mt9m111->width	= mf->width;
> > +	mt9m111->height	= mf->height;
> > +	mt9m111->fmt	= fmt;
> > +
> > +	return 0;
> >  }
> >
> >  static const struct mt9m111_mode_info *
> > @@ -786,14 +781,13 @@ static int mt9m111_s_register(struct v4l2_subdev *sd,
> >  static int mt9m111_set_flip(struct mt9m111 *mt9m111, int flip, int mask)
> >  {
> >  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
> > -	int ret;
> >
> >  	if (flip)
> > -		ret = mt9m111_reg_set(client, mt9m111->ctx->read_mode, mask);
> > +		mt9m111_reg_set(client, mt9m111->ctx->read_mode, mask);
> >  	else
> > -		ret = mt9m111_reg_clear(client, mt9m111->ctx->read_mode, mask);
> > +		mt9m111_reg_clear(client, mt9m111->ctx->read_mode, mask);
> >
> > -	return ret;
> > +	return 0;
> >  }
> >
> >  static int mt9m111_get_global_gain(struct mt9m111 *mt9m111)
> > @@ -823,7 +817,9 @@ static int mt9m111_set_global_gain(struct mt9m111 *mt9m111, int gain)
> >  	else
> >  		val = gain;
> >
> > -	return reg_write(GLOBAL_GAIN, val);
> > +	reg_write(GLOBAL_GAIN, val);
> > +
> > +	return 0;
> >  }
> >
> >  static int mt9m111_set_autoexposure(struct mt9m111 *mt9m111, int val)
> > @@ -831,8 +827,11 @@ static int mt9m111_set_autoexposure(struct mt9m111 *mt9m111, int val)
> >  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
> >
> >  	if (val == V4L2_EXPOSURE_AUTO)
> > -		return reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
> > -	return reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
> > +		reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
> > +	else
> > +		reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
> > +
> > +	return 0;
> >  }
> >
> >  static int mt9m111_set_autowhitebalance(struct mt9m111 *mt9m111, int on)
> > @@ -840,8 +839,11 @@ static int mt9m111_set_autowhitebalance(struct mt9m111 *mt9m111, int on)
> >  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
> >
> >  	if (on)
> > -		return reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
> > -	return reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
> > +		reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
> > +	else
> > +		reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
> > +
> > +	return 0;
> >  }
> >
> >  static const char * const mt9m111_test_pattern_menu[] = {
> > @@ -859,8 +861,9 @@ static int mt9m111_set_test_pattern(struct mt9m111 *mt9m111, int val)
> >  {
> >  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
> >
> > -	return mt9m111_reg_mask(client, MT9M111_TPG_CTRL, val,
> > -				MT9M111_TPG_SEL_MASK);
> > +	mt9m111_reg_mask(client, MT9M111_TPG_CTRL, val, MT9M111_TPG_SEL_MASK);
> > +
> > +	return 0;
> >  }
> >
> >  static int mt9m111_set_colorfx(struct mt9m111 *mt9m111, int val)
> > @@ -877,9 +880,10 @@ static int mt9m111_set_colorfx(struct mt9m111 *mt9m111, int val)
> >
> >  	for (i = 0; i < ARRAY_SIZE(colorfx); i++) {
> >  		if (colorfx[i].id == val) {
> > -			return mt9m111_reg_mask(client, MT9M111_EFFECTS_MODE,
> > -						colorfx[i].value,
> > -						MT9M111_EFFECTS_MODE_MASK);
> > +			mt9m111_reg_mask(client, MT9M111_EFFECTS_MODE,
> > +					 colorfx[i].value,
> > +					 MT9M111_EFFECTS_MODE_MASK);
> > +			return 0;
> >  		}
> >  	}
> >
> > @@ -890,29 +894,41 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
> >  {
> >  	struct mt9m111 *mt9m111 = container_of(ctrl->handler,
> >  					       struct mt9m111, hdl);
> > +	int ret;
> >
> >  	switch (ctrl->id) {
> >  	case V4L2_CID_VFLIP:
> > -		return mt9m111_set_flip(mt9m111, ctrl->val,
> > -					MT9M111_RMB_MIRROR_ROWS);
> > +		ret = mt9m111_set_flip(mt9m111, ctrl->val,
> > +				       MT9M111_RMB_MIRROR_ROWS);
> > +		break;
> >  	case V4L2_CID_HFLIP:
> > -		return mt9m111_set_flip(mt9m111, ctrl->val,
> > -					MT9M111_RMB_MIRROR_COLS);
> > +		ret = mt9m111_set_flip(mt9m111, ctrl->val,
> > +				       MT9M111_RMB_MIRROR_COLS);
> > +		break;
> >  	case V4L2_CID_GAIN:
> > -		return mt9m111_set_global_gain(mt9m111, ctrl->val);
> > +		ret = mt9m111_set_global_gain(mt9m111, ctrl->val);
> > +		break;
> >  	case V4L2_CID_EXPOSURE_AUTO:
> > -		return mt9m111_set_autoexposure(mt9m111, ctrl->val);
> > +		ret = mt9m111_set_autoexposure(mt9m111, ctrl->val);
> > +		break;
> >  	case V4L2_CID_AUTO_WHITE_BALANCE:
> > -		return mt9m111_set_autowhitebalance(mt9m111, ctrl->val);
> > +		ret = mt9m111_set_autowhitebalance(mt9m111, ctrl->val);
> > +		break;
> >  	case V4L2_CID_TEST_PATTERN:
> > -		return mt9m111_set_test_pattern(mt9m111, ctrl->val);
> > +		ret = mt9m111_set_test_pattern(mt9m111, ctrl->val);
> > +		break;
> >  	case V4L2_CID_COLORFX:
> > -		return mt9m111_set_colorfx(mt9m111, ctrl->val);
> > +		ret = mt9m111_set_colorfx(mt9m111, ctrl->val);
> > +		break;
> >  	case V4L2_CID_PIXEL_RATE:
> > -		return 0;
> > +		ret = 0;
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> >  	}
> >
> > -	return -EINVAL;
> > +
> > +	return ret;
> >  }
> >
> >  static int mt9m111_suspend(struct mt9m111 *mt9m111)
> > --
> > 2.30.2
> >
> 

Re: [PATCH 3/4] media: mt9m111: fix device power usage

[Marco Felsch]

Hi Jacopo,

On 22-08-19, Jacopo Mondi wrote:
> Hi Marco
> 
> On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote:
> > Currently the driver turn off the power after probe and toggle it during
> > .stream by using the .s_power callback. This is problematic since other
> > callbacks like .set_fmt accessing the hardware as well which will fail.
> 
> Ouch!
> 
> > So in the end the default format is the only supported format.
> >
> > Remove the hardware register access from the callbacks and instead sync
> > the state once right before the stream gets enabled to fix this.
> 
> Where does it happen in this patch ?

during mt9m111_s_power which gets called by s_power() or after this
small series during s_stream().

> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/media/i2c/mt9m111.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > index 53c4dac4e4bd..cd74c408e110 100644
> > --- a/drivers/media/i2c/mt9m111.c
> > +++ b/drivers/media/i2c/mt9m111.c
> > @@ -481,8 +481,6 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd,
> >  	width = min(mt9m111->width, rect.width);
> >  	height = min(mt9m111->height, rect.height);
> >
> > -
> 
> Why in mainline I don't see these empty lines ?

Hm.. because I introduced this during my "media: mt9m111: fix subdev API
usage" patch.. Sorry.

> > -	mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code);
> >  	mt9m111->rect = rect;
> >  	mt9m111->width = width;
> >  	mt9m111->height = height;
> > @@ -611,7 +609,6 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
> >  	if (mt9m111->pclk_sample == 0)
> >  		mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK;
> >
> > -
> >  	mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
> >  			 data_outfmt2, mask_outfmt2);
> >  	mt9m111_reg_mask(client, context_b.output_fmt_ctrl2,
> > @@ -678,9 +675,6 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
> >  		return 0;
> >  	}
> >
> > -
> > -	mt9m111_setup_geometry(mt9m111, rect, mf->width, mf->height, mf->code);
> > -	mt9m111_set_pixfmt(mt9m111, mf->code);
> 
> Are we looking at two different versions of the driver ??
> https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/mt9m111.c#L684

Same here.

> >  	mt9m111->width	= mf->width;
> >  	mt9m111->height	= mf->height;
> >  	mt9m111->fmt	= fmt;
> > @@ -743,6 +737,8 @@ mt9m111_find_mode(struct mt9m111 *mt9m111, unsigned int req_fps,
> >  	return mode;
> >  }
> >
> > +static int mt9m111_s_power(struct v4l2_subdev *sd, int on);
> > +
> >  #ifdef CONFIG_VIDEO_ADV_DEBUG
> >  static int mt9m111_g_register(struct v4l2_subdev *sd,
> >  			      struct v4l2_dbg_register *reg)
> > @@ -753,10 +749,14 @@ static int mt9m111_g_register(struct v4l2_subdev *sd,
> >  	if (reg->reg > 0x2ff)
> >  		return -EINVAL;
> >
> > +	mt9m111_s_power(sd, 1);
> > +
> >  	val = mt9m111_reg_read(client, reg->reg);
> >  	reg->size = 2;
> >  	reg->val = (u64)val;
> >
> > +	mt9m111_s_power(sd, 0);
> > +
> >  	if (reg->val > 0xffff)
> >  		return -EIO;
> >
> > @@ -771,9 +771,13 @@ static int mt9m111_s_register(struct v4l2_subdev *sd,
> >  	if (reg->reg > 0x2ff)
> >  		return -EINVAL;
> >
> > +	mt9m111_s_power(sd, 1);
> > +
> >  	if (mt9m111_reg_write(client, reg->reg, reg->val) < 0)
> >  		return -EIO;
> >
> > +	mt9m111_s_power(sd, 0);
> > +
> >  	return 0;
> >  }
> >  #endif
> > @@ -896,6 +900,9 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
> >  					       struct mt9m111, hdl);
> >  	int ret;
> >
> > +	if (!mt9m111->is_streaming)
> > +		return 0;
> > +
> >  	switch (ctrl->id) {
> >  	case V4L2_CID_VFLIP:
> >  		ret = mt9m111_set_flip(mt9m111, ctrl->val,
> > @@ -927,7 +934,6 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
> >  		ret = -EINVAL;
> >  	}
> >
> > -
> >  	return ret;
> >  }
> >
> > --
> > 2.30.2
> >
> 

Re: [PATCH 4/4] media: mt9m111: remove .s_power callback

[Jacopo Mondi]

Hi Marco

On Fri, Aug 19, 2022 at 09:18:32AM +0200, Marco Felsch wrote:
> Hi Jacopo,
>
> thanks for your fast feedback :)
>
> On 22-08-18, Jacopo Mondi wrote:
> > Hi Marco
> >
> > On Thu, Aug 18, 2022 at 04:47:12PM +0200, Marco Felsch wrote:
> > > This is in preparation of switching to the generic dev PM mechanism.
> > > Since the .s_power callback will be removed in the near future move the
> > > powering into the .s_stream callback. So this driver no longer depends
> > > on the .s_power mechanism.
> > >
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> >
> > If you want to move to runtime_pm, I would implement it first and have
> > s_power call the _resume and _suspend routines, as some platform
> > drivers still use s_power() and some of them might depend on it.
>
> Do we really have platforms which depend on this? IMHO if that is the

$ git grep "v4l2_subdev_call(.*, s_power" drivers/media/platform/  | cut -d : -f1 | uniq  | wc -l
8

> case than we should fix those platfoms. Since new drivers shouldn't use
> this callback anymore.

Patches are clearly welcome I guess..

>
> In my case, I worked on [1] and wondered why the sensor was enabled
> before I told him to do so. Since I didn't implement the s_power()
> callback, I had no chance to get enabled before.
>

I'm not sure I got this part

> Can we please decide:
>  - Do we wanna get rid of the s_power() callback?

I think that would be everyone's desire, but drivers have to be moved
away from it

>  - If not, how do we handle those devices then with drivers not
>    implementing this callback?

By maintaining compatibility. I suggested to move to runtime_pm() and
wrap _resume/_suspend in s_power(). My understanding is that the two
(runtime_pm/s_power) are mutually exclusive, but even if that was not
the case, runtime_pm is reference counted, hence as long as calls are
balanced this should work, right ?

>
> [1] https://lore.kernel.org/all/20220818143307.967150-1-m.felsch@pengutronix.de/
>
> > It's a slippery slope.. I would love to get rid of s_power() but if
> > any platform uses it with this sensor, it would stop working after
> > this change.
> >
> > > ---
> > >  drivers/media/i2c/mt9m111.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > > index cd74c408e110..8e8ba5a8e6ea 100644
> > > --- a/drivers/media/i2c/mt9m111.c
> > > +++ b/drivers/media/i2c/mt9m111.c
> > > @@ -1065,7 +1065,6 @@ static const struct v4l2_ctrl_ops mt9m111_ctrl_ops = {
> > >  };
> > >
> > >  static const struct v4l2_subdev_core_ops mt9m111_subdev_core_ops = {
> > > -	.s_power	= mt9m111_s_power,
> > >  	.log_status = v4l2_ctrl_subdev_log_status,
> > >  	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> > >  	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
> > > @@ -1136,8 +1135,14 @@ static int mt9m111_enum_mbus_code(struct v4l2_subdev *sd,
> > >  static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable)
> > >  {
> > >  	struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
> > > +	int ret;
> > >
> > >  	mt9m111->is_streaming = !!enable;
> > > +
> > > +	ret = mt9m111_s_power(sd, enable);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	return 0;
> > >  }
> > >
> > > --
> > > 2.30.2
> > >
> >

Re: [PATCH 2/4] media: mt9m111: fix subdev API usage

[Jacopo Mondi]

Hi Marco

On Fri, Aug 19, 2022 at 09:28:04AM +0200, Marco Felsch wrote:
> Hi Jacopo,
>
> On 22-08-19, Jacopo Mondi wrote:
> > Hi Marco
> >
> > On Thu, Aug 18, 2022 at 04:47:10PM +0200, Marco Felsch wrote:
> > > In case of I2C transfer failures the driver return failure codes which
> > > are not allowed according the API documentation. Fix that by ignore the
> > > register access failure codes.
> >
> > I might have missed the reason why subdev ops are not allowed to
> > fail..
>
> Please see the links below:
>
> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-subdev-g-fmt.html?highlight=subdev_s_fmt#return-value
> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-subdev-g-selection.html#return-value
> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-g-ext-ctrls.html#return-value
>
> There is e.g. no ENXIO error code allowed.

Ah ok, my understanding is that you were removing all return values.

I don't think un-checked HW access is a good idea though ?

You should remove HW access from the subdev ops and move the driver to
setup formats/controls at s_stream() time, like you're doing in the
next patches.

>
> > Also you're here changing both subdev ops handler and function that handle
> > controls if I'm not mistaken.
>
> Yes I did that for the controls because the errors are incorrect there
> as well. You're right, I forgot to mention this in the commit message.
>
> Regards,
>   Marco
>
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > >  drivers/media/i2c/mt9m111.c | 116 ++++++++++++++++++++----------------
> > >  1 file changed, 66 insertions(+), 50 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > > index cdaf283e1309..53c4dac4e4bd 100644
> > > --- a/drivers/media/i2c/mt9m111.c
> > > +++ b/drivers/media/i2c/mt9m111.c
> > > @@ -455,7 +455,7 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd,
> > >  	struct mt9m111 *mt9m111 = to_mt9m111(client);
> > >  	struct v4l2_rect rect = sel->r;
> > >  	int width, height;
> > > -	int ret, align = 0;
> > > +	int align = 0;
> > >
> > >  	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE ||
> > >  	    sel->target != V4L2_SEL_TGT_CROP)
> > > @@ -481,14 +481,13 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd,
> > >  	width = min(mt9m111->width, rect.width);
> > >  	height = min(mt9m111->height, rect.height);
> > >
> > > -	ret = mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code);
> > > -	if (!ret) {
> > > -		mt9m111->rect = rect;
> > > -		mt9m111->width = width;
> > > -		mt9m111->height = height;
> > > -	}
> > >
> > > -	return ret;
> > > +	mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code);
> > > +	mt9m111->rect = rect;
> > > +	mt9m111->width = width;
> > > +	mt9m111->height = height;
> > > +
> > > +	return 0;
> > >  }
> > >
> > >  static int mt9m111_get_selection(struct v4l2_subdev *sd,
> > > @@ -558,7 +557,6 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
> > >  		MT9M111_OUTFMT_RGB444x | MT9M111_OUTFMT_RGBx444 |
> > >  		MT9M111_OUTFMT_SWAP_YCbCr_C_Y_RGB_EVEN |
> > >  		MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr_RGB_R_B;
> > > -	int ret;
> > >
> > >  	switch (code) {
> > >  	case MEDIA_BUS_FMT_SBGGR8_1X8:
> > > @@ -613,13 +611,13 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
> > >  	if (mt9m111->pclk_sample == 0)
> > >  		mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK;
> > >
> > > -	ret = mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
> > > -			       data_outfmt2, mask_outfmt2);
> > > -	if (!ret)
> > > -		ret = mt9m111_reg_mask(client, context_b.output_fmt_ctrl2,
> > > -				       data_outfmt2, mask_outfmt2);
> > >
> > > -	return ret;
> > > +	mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
> > > +			 data_outfmt2, mask_outfmt2);
> > > +	mt9m111_reg_mask(client, context_b.output_fmt_ctrl2,
> > > +			 data_outfmt2, mask_outfmt2);
> > > +
> > > +	return 0;
> > >  }
> > >
> > >  static int mt9m111_set_fmt(struct v4l2_subdev *sd,
> > > @@ -632,7 +630,6 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
> > >  	const struct mt9m111_datafmt *fmt;
> > >  	struct v4l2_rect *rect = &mt9m111->rect;
> > >  	bool bayer;
> > > -	int ret;
> > >
> > >  	if (mt9m111->is_streaming)
> > >  		return -EBUSY;
> > > @@ -681,16 +678,14 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
> > >  		return 0;
> > >  	}
> > >
> > > -	ret = mt9m111_setup_geometry(mt9m111, rect, mf->width, mf->height, mf->code);
> > > -	if (!ret)
> > > -		ret = mt9m111_set_pixfmt(mt9m111, mf->code);
> > > -	if (!ret) {
> > > -		mt9m111->width	= mf->width;
> > > -		mt9m111->height	= mf->height;
> > > -		mt9m111->fmt	= fmt;
> > > -	}
> > >
> > > -	return ret;
> > > +	mt9m111_setup_geometry(mt9m111, rect, mf->width, mf->height, mf->code);
> > > +	mt9m111_set_pixfmt(mt9m111, mf->code);
> > > +	mt9m111->width	= mf->width;
> > > +	mt9m111->height	= mf->height;
> > > +	mt9m111->fmt	= fmt;
> > > +
> > > +	return 0;
> > >  }
> > >
> > >  static const struct mt9m111_mode_info *
> > > @@ -786,14 +781,13 @@ static int mt9m111_s_register(struct v4l2_subdev *sd,
> > >  static int mt9m111_set_flip(struct mt9m111 *mt9m111, int flip, int mask)
> > >  {
> > >  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
> > > -	int ret;
> > >
> > >  	if (flip)
> > > -		ret = mt9m111_reg_set(client, mt9m111->ctx->read_mode, mask);
> > > +		mt9m111_reg_set(client, mt9m111->ctx->read_mode, mask);
> > >  	else
> > > -		ret = mt9m111_reg_clear(client, mt9m111->ctx->read_mode, mask);
> > > +		mt9m111_reg_clear(client, mt9m111->ctx->read_mode, mask);
> > >
> > > -	return ret;
> > > +	return 0;
> > >  }
> > >
> > >  static int mt9m111_get_global_gain(struct mt9m111 *mt9m111)
> > > @@ -823,7 +817,9 @@ static int mt9m111_set_global_gain(struct mt9m111 *mt9m111, int gain)
> > >  	else
> > >  		val = gain;
> > >
> > > -	return reg_write(GLOBAL_GAIN, val);
> > > +	reg_write(GLOBAL_GAIN, val);
> > > +
> > > +	return 0;
> > >  }
> > >
> > >  static int mt9m111_set_autoexposure(struct mt9m111 *mt9m111, int val)
> > > @@ -831,8 +827,11 @@ static int mt9m111_set_autoexposure(struct mt9m111 *mt9m111, int val)
> > >  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
> > >
> > >  	if (val == V4L2_EXPOSURE_AUTO)
> > > -		return reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
> > > -	return reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
> > > +		reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
> > > +	else
> > > +		reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
> > > +
> > > +	return 0;
> > >  }
> > >
> > >  static int mt9m111_set_autowhitebalance(struct mt9m111 *mt9m111, int on)
> > > @@ -840,8 +839,11 @@ static int mt9m111_set_autowhitebalance(struct mt9m111 *mt9m111, int on)
> > >  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
> > >
> > >  	if (on)
> > > -		return reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
> > > -	return reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
> > > +		reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
> > > +	else
> > > +		reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
> > > +
> > > +	return 0;
> > >  }
> > >
> > >  static const char * const mt9m111_test_pattern_menu[] = {
> > > @@ -859,8 +861,9 @@ static int mt9m111_set_test_pattern(struct mt9m111 *mt9m111, int val)
> > >  {
> > >  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
> > >
> > > -	return mt9m111_reg_mask(client, MT9M111_TPG_CTRL, val,
> > > -				MT9M111_TPG_SEL_MASK);
> > > +	mt9m111_reg_mask(client, MT9M111_TPG_CTRL, val, MT9M111_TPG_SEL_MASK);
> > > +
> > > +	return 0;
> > >  }
> > >
> > >  static int mt9m111_set_colorfx(struct mt9m111 *mt9m111, int val)
> > > @@ -877,9 +880,10 @@ static int mt9m111_set_colorfx(struct mt9m111 *mt9m111, int val)
> > >
> > >  	for (i = 0; i < ARRAY_SIZE(colorfx); i++) {
> > >  		if (colorfx[i].id == val) {
> > > -			return mt9m111_reg_mask(client, MT9M111_EFFECTS_MODE,
> > > -						colorfx[i].value,
> > > -						MT9M111_EFFECTS_MODE_MASK);
> > > +			mt9m111_reg_mask(client, MT9M111_EFFECTS_MODE,
> > > +					 colorfx[i].value,
> > > +					 MT9M111_EFFECTS_MODE_MASK);
> > > +			return 0;
> > >  		}
> > >  	}
> > >
> > > @@ -890,29 +894,41 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
> > >  {
> > >  	struct mt9m111 *mt9m111 = container_of(ctrl->handler,
> > >  					       struct mt9m111, hdl);
> > > +	int ret;
> > >
> > >  	switch (ctrl->id) {
> > >  	case V4L2_CID_VFLIP:
> > > -		return mt9m111_set_flip(mt9m111, ctrl->val,
> > > -					MT9M111_RMB_MIRROR_ROWS);
> > > +		ret = mt9m111_set_flip(mt9m111, ctrl->val,
> > > +				       MT9M111_RMB_MIRROR_ROWS);
> > > +		break;
> > >  	case V4L2_CID_HFLIP:
> > > -		return mt9m111_set_flip(mt9m111, ctrl->val,
> > > -					MT9M111_RMB_MIRROR_COLS);
> > > +		ret = mt9m111_set_flip(mt9m111, ctrl->val,
> > > +				       MT9M111_RMB_MIRROR_COLS);
> > > +		break;
> > >  	case V4L2_CID_GAIN:
> > > -		return mt9m111_set_global_gain(mt9m111, ctrl->val);
> > > +		ret = mt9m111_set_global_gain(mt9m111, ctrl->val);
> > > +		break;
> > >  	case V4L2_CID_EXPOSURE_AUTO:
> > > -		return mt9m111_set_autoexposure(mt9m111, ctrl->val);
> > > +		ret = mt9m111_set_autoexposure(mt9m111, ctrl->val);
> > > +		break;
> > >  	case V4L2_CID_AUTO_WHITE_BALANCE:
> > > -		return mt9m111_set_autowhitebalance(mt9m111, ctrl->val);
> > > +		ret = mt9m111_set_autowhitebalance(mt9m111, ctrl->val);
> > > +		break;
> > >  	case V4L2_CID_TEST_PATTERN:
> > > -		return mt9m111_set_test_pattern(mt9m111, ctrl->val);
> > > +		ret = mt9m111_set_test_pattern(mt9m111, ctrl->val);
> > > +		break;
> > >  	case V4L2_CID_COLORFX:
> > > -		return mt9m111_set_colorfx(mt9m111, ctrl->val);
> > > +		ret = mt9m111_set_colorfx(mt9m111, ctrl->val);
> > > +		break;
> > >  	case V4L2_CID_PIXEL_RATE:
> > > -		return 0;
> > > +		ret = 0;
> > > +		break;
> > > +	default:
> > > +		ret = -EINVAL;
> > >  	}
> > >
> > > -	return -EINVAL;
> > > +
> > > +	return ret;
> > >  }
> > >
> > >  static int mt9m111_suspend(struct mt9m111 *mt9m111)
> > > --
> > > 2.30.2
> > >
> >

Re: [PATCH 1/4] media: mt9m111: add V4L2_CID_PIXEL_RATE support

[Marco Felsch]

Hi Jacopo,

On 22-08-18, Jacopo Mondi wrote:
> Hi Marco
> 
> On Thu, Aug 18, 2022 at 04:47:09PM +0200, Marco Felsch wrote:
> > Add support to report the PIXEL_RATE so a host or bridge device can ask
> > the sensor.
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/media/i2c/mt9m111.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > index afc86efa9e3e..cdaf283e1309 100644
> > --- a/drivers/media/i2c/mt9m111.c
> > +++ b/drivers/media/i2c/mt9m111.c
> > @@ -908,6 +908,8 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
> >  		return mt9m111_set_test_pattern(mt9m111, ctrl->val);
> >  	case V4L2_CID_COLORFX:
> >  		return mt9m111_set_colorfx(mt9m111, ctrl->val);
> > +	case V4L2_CID_PIXEL_RATE:
> > +		return 0;
> 
> By default PIXEL_RATE is read-only.
> Do you get a call to s_ctrl for it ?

You're absolutly right, we don't need to do this.

> >  	}
> >
> >  	return -EINVAL;
> > @@ -1249,6 +1251,7 @@ static int mt9m111_probe(struct i2c_client *client)
> >  {
> >  	struct mt9m111 *mt9m111;
> >  	struct i2c_adapter *adapter = client->adapter;
> > +	unsigned long mclk_rate;
> >  	int ret;
> >
> >  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
> > @@ -1271,6 +1274,13 @@ static int mt9m111_probe(struct i2c_client *client)
> >  	if (IS_ERR(mt9m111->clk))
> >  		return PTR_ERR(mt9m111->clk);
> >
> > +	ret = clk_prepare_enable(mt9m111->clk);
> > +	if (ret < 0)
> > +		return ret;
> > +
> 
> Do you need to enable clock to read its rate ?

Yes, accroding the API [1].

[1] https://elixir.bootlin.com/linux/v6.0-rc1/source/include/linux/clk.h#L682

> > +	mclk_rate = clk_get_rate(mt9m111->clk);
> > +	clk_disable_unprepare(mt9m111->clk);
> > +
> >  	mt9m111->regulator = devm_regulator_get(&client->dev, "vdd");
> >  	if (IS_ERR(mt9m111->regulator)) {
> >  		dev_err(&client->dev, "regulator not found: %ld\n",
> > @@ -1285,7 +1295,7 @@ static int mt9m111_probe(struct i2c_client *client)
> >  	mt9m111->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> >  				 V4L2_SUBDEV_FL_HAS_EVENTS;
> >
> > -	v4l2_ctrl_handler_init(&mt9m111->hdl, 7);
> > +	v4l2_ctrl_handler_init(&mt9m111->hdl, 8);
> >  	v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> >  			V4L2_CID_VFLIP, 0, 1, 1, 0);
> >  	v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> > @@ -1309,6 +1319,9 @@ static int mt9m111_probe(struct i2c_client *client)
> >  				BIT(V4L2_COLORFX_NEGATIVE) |
> >  				BIT(V4L2_COLORFX_SOLARIZATION)),
> >  			V4L2_COLORFX_NONE);
> > +	v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops, V4L2_CID_PIXEL_RATE,
> > +			  mclk_rate, mclk_rate, 1, mclk_rate);
> > +
> 
> I don't have a datasheet but it seems a little weird that the mclk
> frequency is the same as the pixel clock rate ?

I see your confusion here. I can only speak for the MT9M131 device which
is covered by this driver as well. This device is composed of a
internal-sensor and a internal-isp. The internal-sensor is clocked by
mclk/2 but the final image device/sensor output-fifo is clocked by mclk.
There are options to devide the output clock as well, but these options
are not enabled yet. So yes, the pixel clock rate == mclk rate. To avoid
confusion I could rename the mclk_rate to extclk_rate but then clock
request is not 100% correct since we are requesting a "mclk", this
should be "extclk".

Regards,
  Marco

> >  	mt9m111->subdev.ctrl_handler = &mt9m111->hdl;
> >  	if (mt9m111->hdl.error) {
> >  		ret = mt9m111->hdl.error;
> > --
> > 2.30.2
> >
> 

Re: [PATCH 4/4] media: mt9m111: remove .s_power callback

[Marco Felsch]

On 22-08-19, Jacopo Mondi wrote:
> Hi Marco
> 
> On Fri, Aug 19, 2022 at 09:18:32AM +0200, Marco Felsch wrote:
> > Hi Jacopo,
> >
> > thanks for your fast feedback :)
> >
> > On 22-08-18, Jacopo Mondi wrote:
> > > Hi Marco
> > >
> > > On Thu, Aug 18, 2022 at 04:47:12PM +0200, Marco Felsch wrote:
> > > > This is in preparation of switching to the generic dev PM mechanism.
> > > > Since the .s_power callback will be removed in the near future move the
> > > > powering into the .s_stream callback. So this driver no longer depends
> > > > on the .s_power mechanism.
> > > >
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > >
> > > If you want to move to runtime_pm, I would implement it first and have
> > > s_power call the _resume and _suspend routines, as some platform
> > > drivers still use s_power() and some of them might depend on it.
> >
> > Do we really have platforms which depend on this? IMHO if that is the
> 
> $ git grep "v4l2_subdev_call(.*, s_power" drivers/media/platform/  | cut -d : -f1 | uniq  | wc -l
> 8
> 
> > case than we should fix those platfoms. Since new drivers shouldn't use
> > this callback anymore.
> 
> Patches are clearly welcome I guess..

:)

> > In my case, I worked on [1] and wondered why the sensor was enabled
> > before I told him to do so. Since I didn't implement the s_power()
> > callback, I had no chance to get enabled before.
> >
> 
> I'm not sure I got this part

What I mean is, that the MT9M131 sensor gets enabled and immediately
start sending frames before I told him to do so e.g. by calling
s_stream(). This can confuse the downstream device. The only way to get
enable the downstream device first is to add the s_power() callback.

> > Can we please decide:
> >  - Do we wanna get rid of the s_power() callback?
> 
> I think that would be everyone's desire, but drivers have to be moved
> away from it
> 
> >  - If not, how do we handle those devices then with drivers not
> >    implementing this callback?
> 
> By maintaining compatibility. I suggested to move to runtime_pm() and
> wrap _resume/_suspend in s_power(). 

But then you're introducing new drivers with s_power() callbacks and so
the behaviour isn't really changed.

> My understanding is that the two (runtime_pm/s_power) are mutually
> exclusive, but even if that was not the case, runtime_pm is reference
> counted, hence as long as calls are balanced this should work, right ?

Right but the s_power() behaviour is not changed and drivers still rely
on it to work as right now.

Regards,
  Marco

Re: [PATCH 1/4] media: mt9m111: add V4L2_CID_PIXEL_RATE support

[Jacopo Mondi]

Hi Marco

On Fri, Aug 19, 2022 at 09:56:15AM +0200, Marco Felsch wrote:
> Hi Jacopo,
>
> On 22-08-18, Jacopo Mondi wrote:
> > Hi Marco
> >
> > On Thu, Aug 18, 2022 at 04:47:09PM +0200, Marco Felsch wrote:
> > > Add support to report the PIXEL_RATE so a host or bridge device can ask
> > > the sensor.
> > >
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > >  drivers/media/i2c/mt9m111.c | 15 ++++++++++++++-
> > >  1 file changed, 14 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > > index afc86efa9e3e..cdaf283e1309 100644
> > > --- a/drivers/media/i2c/mt9m111.c
> > > +++ b/drivers/media/i2c/mt9m111.c
> > > @@ -908,6 +908,8 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
> > >  		return mt9m111_set_test_pattern(mt9m111, ctrl->val);
> > >  	case V4L2_CID_COLORFX:
> > >  		return mt9m111_set_colorfx(mt9m111, ctrl->val);
> > > +	case V4L2_CID_PIXEL_RATE:
> > > +		return 0;
> >
> > By default PIXEL_RATE is read-only.
> > Do you get a call to s_ctrl for it ?
>
> You're absolutly right, we don't need to do this.
>
> > >  	}
> > >
> > >  	return -EINVAL;
> > > @@ -1249,6 +1251,7 @@ static int mt9m111_probe(struct i2c_client *client)
> > >  {
> > >  	struct mt9m111 *mt9m111;
> > >  	struct i2c_adapter *adapter = client->adapter;
> > > +	unsigned long mclk_rate;
> > >  	int ret;
> > >
> > >  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
> > > @@ -1271,6 +1274,13 @@ static int mt9m111_probe(struct i2c_client *client)
> > >  	if (IS_ERR(mt9m111->clk))
> > >  		return PTR_ERR(mt9m111->clk);
> > >
> > > +	ret = clk_prepare_enable(mt9m111->clk);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> >
> > Do you need to enable clock to read its rate ?
>
> Yes, accroding the API [1].
>
> [1] https://elixir.bootlin.com/linux/v6.0-rc1/source/include/linux/clk.h#L682

So weird! none of the drivers I checked do that. The most common
pattern is

        clk = devm_clk_get();
        rate = clk_get_rate(clk)
        if (rate != RATE)
                ...

>
> > > +	mclk_rate = clk_get_rate(mt9m111->clk);
> > > +	clk_disable_unprepare(mt9m111->clk);
> > > +
> > >  	mt9m111->regulator = devm_regulator_get(&client->dev, "vdd");
> > >  	if (IS_ERR(mt9m111->regulator)) {
> > >  		dev_err(&client->dev, "regulator not found: %ld\n",
> > > @@ -1285,7 +1295,7 @@ static int mt9m111_probe(struct i2c_client *client)
> > >  	mt9m111->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> > >  				 V4L2_SUBDEV_FL_HAS_EVENTS;
> > >
> > > -	v4l2_ctrl_handler_init(&mt9m111->hdl, 7);
> > > +	v4l2_ctrl_handler_init(&mt9m111->hdl, 8);
> > >  	v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> > >  			V4L2_CID_VFLIP, 0, 1, 1, 0);
> > >  	v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> > > @@ -1309,6 +1319,9 @@ static int mt9m111_probe(struct i2c_client *client)
> > >  				BIT(V4L2_COLORFX_NEGATIVE) |
> > >  				BIT(V4L2_COLORFX_SOLARIZATION)),
> > >  			V4L2_COLORFX_NONE);
> > > +	v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops, V4L2_CID_PIXEL_RATE,
> > > +			  mclk_rate, mclk_rate, 1, mclk_rate);
> > > +
> >
> > I don't have a datasheet but it seems a little weird that the mclk
> > frequency is the same as the pixel clock rate ?
>
> I see your confusion here. I can only speak for the MT9M131 device which
> is covered by this driver as well. This device is composed of a
> internal-sensor and a internal-isp. The internal-sensor is clocked by
> mclk/2 but the final image device/sensor output-fifo is clocked by mclk.

No PLL, no rate multiplier/divider ? Does this sensor only work with
one pixel rate that is defined by the input clock rate ?

> There are options to devide the output clock as well, but these options
> are not enabled yet. So yes, the pixel clock rate == mclk rate. To avoid

Ah ok

Could you share your setup mclk, resolution and frame rate to show how
the pixel rate and the mclk rate are related ?

> confusion I could rename the mclk_rate to extclk_rate but then clock
> request is not 100% correct since we are requesting a "mclk", this
> should be "extclk".

Doesn't really make a difference!

A comment in the code to explain what happens would help though!

>
> Regards,
>   Marco
>
> > >  	mt9m111->subdev.ctrl_handler = &mt9m111->hdl;
> > >  	if (mt9m111->hdl.error) {
> > >  		ret = mt9m111->hdl.error;
> > > --
> > > 2.30.2
> > >
> >

Re: [PATCH 4/4] media: mt9m111: remove .s_power callback

[Jacopo Mondi]

Hi Marco

On Fri, Aug 19, 2022 at 10:06:26AM +0200, Marco Felsch wrote:
> On 22-08-19, Jacopo Mondi wrote:
> > Hi Marco
> >
> > On Fri, Aug 19, 2022 at 09:18:32AM +0200, Marco Felsch wrote:
> > > Hi Jacopo,
> > >
> > > thanks for your fast feedback :)
> > >
> > > On 22-08-18, Jacopo Mondi wrote:
> > > > Hi Marco
> > > >
> > > > On Thu, Aug 18, 2022 at 04:47:12PM +0200, Marco Felsch wrote:
> > > > > This is in preparation of switching to the generic dev PM mechanism.
> > > > > Since the .s_power callback will be removed in the near future move the
> > > > > powering into the .s_stream callback. So this driver no longer depends
> > > > > on the .s_power mechanism.
> > > > >
> > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > >
> > > > If you want to move to runtime_pm, I would implement it first and have
> > > > s_power call the _resume and _suspend routines, as some platform
> > > > drivers still use s_power() and some of them might depend on it.
> > >
> > > Do we really have platforms which depend on this? IMHO if that is the
> >
> > $ git grep "v4l2_subdev_call(.*, s_power" drivers/media/platform/  | cut -d : -f1 | uniq  | wc -l
> > 8
> >
> > > case than we should fix those platfoms. Since new drivers shouldn't use
> > > this callback anymore.
> >
> > Patches are clearly welcome I guess..
>
> :)
>
> > > In my case, I worked on [1] and wondered why the sensor was enabled
> > > before I told him to do so. Since I didn't implement the s_power()
> > > callback, I had no chance to get enabled before.
> > >
> >
> > I'm not sure I got this part
>
> What I mean is, that the MT9M131 sensor gets enabled and immediately
> start sending frames before I told him to do so e.g. by calling

Why does this happen ?

> s_stream(). This can confuse the downstream device. The only way to get
> enable the downstream device first is to add the s_power() callback.
>
> > > Can we please decide:
> > >  - Do we wanna get rid of the s_power() callback?
> >
> > I think that would be everyone's desire, but drivers have to be moved
> > away from it
> >
> > >  - If not, how do we handle those devices then with drivers not
> > >    implementing this callback?
> >
> > By maintaining compatibility. I suggested to move to runtime_pm() and
> > wrap _resume/_suspend in s_power().
>
> But then you're introducing new drivers with s_power() callbacks and so
> the behaviour isn't really changed.
>

I only meant in existing ones

> > My understanding is that the two (runtime_pm/s_power) are mutually
> > exclusive, but even if that was not the case, runtime_pm is reference
> > counted, hence as long as calls are balanced this should work, right ?
>
> Right but the s_power() behaviour is not changed and drivers still rely
> on it to work as right now.

As long as we have bridge drivers using it, isn't this what we want ?
>
> Regards,
>   Marco

Re: [PATCH 1/4] media: mt9m111: add V4L2_CID_PIXEL_RATE support

[Marco Felsch]

On 22-08-19, Jacopo Mondi wrote:
> Hi Marco
> 
> On Fri, Aug 19, 2022 at 09:56:15AM +0200, Marco Felsch wrote:
> > Hi Jacopo,
> >
> > On 22-08-18, Jacopo Mondi wrote:
> > > Hi Marco
> > >
> > > On Thu, Aug 18, 2022 at 04:47:09PM +0200, Marco Felsch wrote:
> > > > Add support to report the PIXEL_RATE so a host or bridge device can ask
> > > > the sensor.
> > > >
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > ---
> > > >  drivers/media/i2c/mt9m111.c | 15 ++++++++++++++-
> > > >  1 file changed, 14 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > > > index afc86efa9e3e..cdaf283e1309 100644
> > > > --- a/drivers/media/i2c/mt9m111.c
> > > > +++ b/drivers/media/i2c/mt9m111.c
> > > > @@ -908,6 +908,8 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
> > > >  		return mt9m111_set_test_pattern(mt9m111, ctrl->val);
> > > >  	case V4L2_CID_COLORFX:
> > > >  		return mt9m111_set_colorfx(mt9m111, ctrl->val);
> > > > +	case V4L2_CID_PIXEL_RATE:
> > > > +		return 0;
> > >
> > > By default PIXEL_RATE is read-only.
> > > Do you get a call to s_ctrl for it ?
> >
> > You're absolutly right, we don't need to do this.
> >
> > > >  	}
> > > >
> > > >  	return -EINVAL;
> > > > @@ -1249,6 +1251,7 @@ static int mt9m111_probe(struct i2c_client *client)
> > > >  {
> > > >  	struct mt9m111 *mt9m111;
> > > >  	struct i2c_adapter *adapter = client->adapter;
> > > > +	unsigned long mclk_rate;
> > > >  	int ret;
> > > >
> > > >  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
> > > > @@ -1271,6 +1274,13 @@ static int mt9m111_probe(struct i2c_client *client)
> > > >  	if (IS_ERR(mt9m111->clk))
> > > >  		return PTR_ERR(mt9m111->clk);
> > > >
> > > > +	ret = clk_prepare_enable(mt9m111->clk);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > >
> > > Do you need to enable clock to read its rate ?
> >
> > Yes, accroding the API [1].
> >
> > [1] https://elixir.bootlin.com/linux/v6.0-rc1/source/include/linux/clk.h#L682
> 
> So weird! none of the drivers I checked do that. The most common
> pattern is
> 
>         clk = devm_clk_get();
>         rate = clk_get_rate(clk)
>         if (rate != RATE)
>                 ...

Yep, I know. There are a lot of drivers not respecting this.

> >
> > > > +	mclk_rate = clk_get_rate(mt9m111->clk);
> > > > +	clk_disable_unprepare(mt9m111->clk);
> > > > +
> > > >  	mt9m111->regulator = devm_regulator_get(&client->dev, "vdd");
> > > >  	if (IS_ERR(mt9m111->regulator)) {
> > > >  		dev_err(&client->dev, "regulator not found: %ld\n",
> > > > @@ -1285,7 +1295,7 @@ static int mt9m111_probe(struct i2c_client *client)
> > > >  	mt9m111->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> > > >  				 V4L2_SUBDEV_FL_HAS_EVENTS;
> > > >
> > > > -	v4l2_ctrl_handler_init(&mt9m111->hdl, 7);
> > > > +	v4l2_ctrl_handler_init(&mt9m111->hdl, 8);
> > > >  	v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> > > >  			V4L2_CID_VFLIP, 0, 1, 1, 0);
> > > >  	v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> > > > @@ -1309,6 +1319,9 @@ static int mt9m111_probe(struct i2c_client *client)
> > > >  				BIT(V4L2_COLORFX_NEGATIVE) |
> > > >  				BIT(V4L2_COLORFX_SOLARIZATION)),
> > > >  			V4L2_COLORFX_NONE);
> > > > +	v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops, V4L2_CID_PIXEL_RATE,
> > > > +			  mclk_rate, mclk_rate, 1, mclk_rate);
> > > > +
> > >
> > > I don't have a datasheet but it seems a little weird that the mclk
> > > frequency is the same as the pixel clock rate ?
> >
> > I see your confusion here. I can only speak for the MT9M131 device which
> > is covered by this driver as well. This device is composed of a
> > internal-sensor and a internal-isp. The internal-sensor is clocked by
> > mclk/2 but the final image device/sensor output-fifo is clocked by mclk.
> 
> No PLL, no rate multiplier/divider ? Does this sensor only work with
> one pixel rate that is defined by the input clock rate ?
> 
> > There are options to devide the output clock as well, but these options
> > are not enabled yet. So yes, the pixel clock rate == mclk rate. To avoid
> 
> Ah ok
> 
> Could you share your setup mclk, resolution and frame rate to show how
> the pixel rate and the mclk rate are related ?


mclk:      54 MHz (input)
pixelclk:  54 MHz (output)
res:       1280x1024
fps:       15
bus-width: 8
bpp:       16

After re-reading the PIXEL_RATE, maybe I missunderstood the control. As
of now I thought it is the amount of bytes per second send on the bus.
But the documentation says pixels per second... Is there a control to
expose the current pixelclk on the mbus? What I wanna do in the end is
to calculate the througput on the (parallel-)bus.

> > confusion I could rename the mclk_rate to extclk_rate but then clock
> > request is not 100% correct since we are requesting a "mclk", this
> > should be "extclk".
> 
> Doesn't really make a difference!
> 
> A comment in the code to explain what happens would help though!

I did that right now, after your confusion :)

Regards,
  Marco

> 
> >
> > Regards,
> >   Marco
> >
> > > >  	mt9m111->subdev.ctrl_handler = &mt9m111->hdl;
> > > >  	if (mt9m111->hdl.error) {
> > > >  		ret = mt9m111->hdl.error;
> > > > --
> > > > 2.30.2
> > > >
> > >
> 

Re: [PATCH 1/4] media: mt9m111: add V4L2_CID_PIXEL_RATE support

[Jacopo Mondi]

Hi Marco,

On Fri, Aug 19, 2022 at 11:04:38AM +0200, Marco Felsch wrote:
> On 22-08-19, Jacopo Mondi wrote:
> > Hi Marco
> >
> > On Fri, Aug 19, 2022 at 09:56:15AM +0200, Marco Felsch wrote:
> > > Hi Jacopo,
> > >
> > > On 22-08-18, Jacopo Mondi wrote:
> > > > Hi Marco
> > > >
> > > > On Thu, Aug 18, 2022 at 04:47:09PM +0200, Marco Felsch wrote:
> > > > > Add support to report the PIXEL_RATE so a host or bridge device can ask
> > > > > the sensor.
> > > > >
> > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > > ---
> > > > >  drivers/media/i2c/mt9m111.c | 15 ++++++++++++++-
> > > > >  1 file changed, 14 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > > > > index afc86efa9e3e..cdaf283e1309 100644
> > > > > --- a/drivers/media/i2c/mt9m111.c
> > > > > +++ b/drivers/media/i2c/mt9m111.c
> > > > > @@ -908,6 +908,8 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
> > > > >  		return mt9m111_set_test_pattern(mt9m111, ctrl->val);
> > > > >  	case V4L2_CID_COLORFX:
> > > > >  		return mt9m111_set_colorfx(mt9m111, ctrl->val);
> > > > > +	case V4L2_CID_PIXEL_RATE:
> > > > > +		return 0;
> > > >
> > > > By default PIXEL_RATE is read-only.
> > > > Do you get a call to s_ctrl for it ?
> > >
> > > You're absolutly right, we don't need to do this.
> > >
> > > > >  	}
> > > > >
> > > > >  	return -EINVAL;
> > > > > @@ -1249,6 +1251,7 @@ static int mt9m111_probe(struct i2c_client *client)
> > > > >  {
> > > > >  	struct mt9m111 *mt9m111;
> > > > >  	struct i2c_adapter *adapter = client->adapter;
> > > > > +	unsigned long mclk_rate;
> > > > >  	int ret;
> > > > >
> > > > >  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
> > > > > @@ -1271,6 +1274,13 @@ static int mt9m111_probe(struct i2c_client *client)
> > > > >  	if (IS_ERR(mt9m111->clk))
> > > > >  		return PTR_ERR(mt9m111->clk);
> > > > >
> > > > > +	ret = clk_prepare_enable(mt9m111->clk);
> > > > > +	if (ret < 0)
> > > > > +		return ret;
> > > > > +
> > > >
> > > > Do you need to enable clock to read its rate ?
> > >
> > > Yes, accroding the API [1].
> > >
> > > [1] https://elixir.bootlin.com/linux/v6.0-rc1/source/include/linux/clk.h#L682
> >
> > So weird! none of the drivers I checked do that. The most common
> > pattern is
> >
> >         clk = devm_clk_get();
> >         rate = clk_get_rate(clk)
> >         if (rate != RATE)
> >                 ...
>
> Yep, I know. There are a lot of drivers not respecting this.
>

I wonder if it's really necessary then :)

> > >
> > > > > +	mclk_rate = clk_get_rate(mt9m111->clk);
> > > > > +	clk_disable_unprepare(mt9m111->clk);
> > > > > +
> > > > >  	mt9m111->regulator = devm_regulator_get(&client->dev, "vdd");
> > > > >  	if (IS_ERR(mt9m111->regulator)) {
> > > > >  		dev_err(&client->dev, "regulator not found: %ld\n",
> > > > > @@ -1285,7 +1295,7 @@ static int mt9m111_probe(struct i2c_client *client)
> > > > >  	mt9m111->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> > > > >  				 V4L2_SUBDEV_FL_HAS_EVENTS;
> > > > >
> > > > > -	v4l2_ctrl_handler_init(&mt9m111->hdl, 7);
> > > > > +	v4l2_ctrl_handler_init(&mt9m111->hdl, 8);
> > > > >  	v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> > > > >  			V4L2_CID_VFLIP, 0, 1, 1, 0);
> > > > >  	v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> > > > > @@ -1309,6 +1319,9 @@ static int mt9m111_probe(struct i2c_client *client)
> > > > >  				BIT(V4L2_COLORFX_NEGATIVE) |
> > > > >  				BIT(V4L2_COLORFX_SOLARIZATION)),
> > > > >  			V4L2_COLORFX_NONE);
> > > > > +	v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops, V4L2_CID_PIXEL_RATE,
> > > > > +			  mclk_rate, mclk_rate, 1, mclk_rate);
> > > > > +
> > > >
> > > > I don't have a datasheet but it seems a little weird that the mclk
> > > > frequency is the same as the pixel clock rate ?
> > >
> > > I see your confusion here. I can only speak for the MT9M131 device which
> > > is covered by this driver as well. This device is composed of a
> > > internal-sensor and a internal-isp. The internal-sensor is clocked by
> > > mclk/2 but the final image device/sensor output-fifo is clocked by mclk.
> >
> > No PLL, no rate multiplier/divider ? Does this sensor only work with
> > one pixel rate that is defined by the input clock rate ?
> >
> > > There are options to devide the output clock as well, but these options
> > > are not enabled yet. So yes, the pixel clock rate == mclk rate. To avoid
> >
> > Ah ok
> >
> > Could you share your setup mclk, resolution and frame rate to show how
> > the pixel rate and the mclk rate are related ?
>
>
> mclk:      54 MHz (input)
> pixelclk:  54 MHz (output)
> res:       1280x1024

You should take blankings into account as well, even if I havent'
found where they are programmed in the driver


> fps:       15
> bus-width: 8
> bpp:       16
>
> After re-reading the PIXEL_RATE, maybe I missunderstood the control. As
> of now I thought it is the amount of bytes per second send on the bus.

Not bytes but pixels :)

And the above gives me a 19.660.800 Hz pixel rate


> But the documentation says pixels per second... Is there a control to
> expose the current pixelclk on the mbus? What I wanna do in the end is
> to calculate the througput on the (parallel-)bus.

Not for parallel busses as far as I'm aware as my understanding is
that CID_LINK_FREQ applies to CSI-2 setups only.

To get the byte rate on the bus I would simply multiply the pixel
clock by the number of bytes per pixel, in this case 2.

>
> > > confusion I could rename the mclk_rate to extclk_rate but then clock
> > > request is not 100% correct since we are requesting a "mclk", this
> > > should be "extclk".
> >
> > Doesn't really make a difference!
> >
> > A comment in the code to explain what happens would help though!
>
> I did that right now, after your confusion :)
>
> Regards,
>   Marco
>
> >
> > >
> > > Regards,
> > >   Marco
> > >
> > > > >  	mt9m111->subdev.ctrl_handler = &mt9m111->hdl;
> > > > >  	if (mt9m111->hdl.error) {
> > > > >  		ret = mt9m111->hdl.error;
> > > > > --
> > > > > 2.30.2
> > > > >
> > > >
> >

Re: [PATCH 1/4] media: mt9m111: add V4L2_CID_PIXEL_RATE support

[Marco Felsch]

On 22-08-19, Jacopo Mondi wrote:
> Hi Marco,
> 
> On Fri, Aug 19, 2022 at 11:04:38AM +0200, Marco Felsch wrote:
> > On 22-08-19, Jacopo Mondi wrote:
> > > Hi Marco
> > >
> > > On Fri, Aug 19, 2022 at 09:56:15AM +0200, Marco Felsch wrote:
> > > > Hi Jacopo,
> > > >
> > > > On 22-08-18, Jacopo Mondi wrote:
> > > > > Hi Marco
> > > > >
> > > > > On Thu, Aug 18, 2022 at 04:47:09PM +0200, Marco Felsch wrote:
> > > > > > Add support to report the PIXEL_RATE so a host or bridge device can ask
> > > > > > the sensor.
> > > > > >
> > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > > > ---
> > > > > >  drivers/media/i2c/mt9m111.c | 15 ++++++++++++++-
> > > > > >  1 file changed, 14 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > > > > > index afc86efa9e3e..cdaf283e1309 100644
> > > > > > --- a/drivers/media/i2c/mt9m111.c
> > > > > > +++ b/drivers/media/i2c/mt9m111.c
> > > > > > @@ -908,6 +908,8 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
> > > > > >  		return mt9m111_set_test_pattern(mt9m111, ctrl->val);
> > > > > >  	case V4L2_CID_COLORFX:
> > > > > >  		return mt9m111_set_colorfx(mt9m111, ctrl->val);
> > > > > > +	case V4L2_CID_PIXEL_RATE:
> > > > > > +		return 0;
> > > > >
> > > > > By default PIXEL_RATE is read-only.
> > > > > Do you get a call to s_ctrl for it ?
> > > >
> > > > You're absolutly right, we don't need to do this.
> > > >
> > > > > >  	}
> > > > > >
> > > > > >  	return -EINVAL;
> > > > > > @@ -1249,6 +1251,7 @@ static int mt9m111_probe(struct i2c_client *client)
> > > > > >  {
> > > > > >  	struct mt9m111 *mt9m111;
> > > > > >  	struct i2c_adapter *adapter = client->adapter;
> > > > > > +	unsigned long mclk_rate;
> > > > > >  	int ret;
> > > > > >
> > > > > >  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
> > > > > > @@ -1271,6 +1274,13 @@ static int mt9m111_probe(struct i2c_client *client)
> > > > > >  	if (IS_ERR(mt9m111->clk))
> > > > > >  		return PTR_ERR(mt9m111->clk);
> > > > > >
> > > > > > +	ret = clk_prepare_enable(mt9m111->clk);
> > > > > > +	if (ret < 0)
> > > > > > +		return ret;
> > > > > > +
> > > > >
> > > > > Do you need to enable clock to read its rate ?
> > > >
> > > > Yes, accroding the API [1].
> > > >
> > > > [1] https://elixir.bootlin.com/linux/v6.0-rc1/source/include/linux/clk.h#L682
> > >
> > > So weird! none of the drivers I checked do that. The most common
> > > pattern is
> > >
> > >         clk = devm_clk_get();
> > >         rate = clk_get_rate(clk)
> > >         if (rate != RATE)
> > >                 ...
> >
> > Yep, I know. There are a lot of drivers not respecting this.
> >
> 
> I wonder if it's really necessary then :)

I would rather keep in sync with the official API ^^

> > > >
> > > > > > +	mclk_rate = clk_get_rate(mt9m111->clk);
> > > > > > +	clk_disable_unprepare(mt9m111->clk);
> > > > > > +
> > > > > >  	mt9m111->regulator = devm_regulator_get(&client->dev, "vdd");
> > > > > >  	if (IS_ERR(mt9m111->regulator)) {
> > > > > >  		dev_err(&client->dev, "regulator not found: %ld\n",
> > > > > > @@ -1285,7 +1295,7 @@ static int mt9m111_probe(struct i2c_client *client)
> > > > > >  	mt9m111->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> > > > > >  				 V4L2_SUBDEV_FL_HAS_EVENTS;
> > > > > >
> > > > > > -	v4l2_ctrl_handler_init(&mt9m111->hdl, 7);
> > > > > > +	v4l2_ctrl_handler_init(&mt9m111->hdl, 8);
> > > > > >  	v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> > > > > >  			V4L2_CID_VFLIP, 0, 1, 1, 0);
> > > > > >  	v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
> > > > > > @@ -1309,6 +1319,9 @@ static int mt9m111_probe(struct i2c_client *client)
> > > > > >  				BIT(V4L2_COLORFX_NEGATIVE) |
> > > > > >  				BIT(V4L2_COLORFX_SOLARIZATION)),
> > > > > >  			V4L2_COLORFX_NONE);
> > > > > > +	v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops, V4L2_CID_PIXEL_RATE,
> > > > > > +			  mclk_rate, mclk_rate, 1, mclk_rate);
> > > > > > +
> > > > >
> > > > > I don't have a datasheet but it seems a little weird that the mclk
> > > > > frequency is the same as the pixel clock rate ?
> > > >
> > > > I see your confusion here. I can only speak for the MT9M131 device which
> > > > is covered by this driver as well. This device is composed of a
> > > > internal-sensor and a internal-isp. The internal-sensor is clocked by
> > > > mclk/2 but the final image device/sensor output-fifo is clocked by mclk.
> > >
> > > No PLL, no rate multiplier/divider ? Does this sensor only work with
> > > one pixel rate that is defined by the input clock rate ?
> > >
> > > > There are options to devide the output clock as well, but these options
> > > > are not enabled yet. So yes, the pixel clock rate == mclk rate. To avoid
> > >
> > > Ah ok
> > >
> > > Could you share your setup mclk, resolution and frame rate to show how
> > > the pixel rate and the mclk rate are related ?
> >
> >
> > mclk:      54 MHz (input)
> > pixelclk:  54 MHz (output)
> > res:       1280x1024
> 
> You should take blankings into account as well, even if I havent'
> found where they are programmed in the driver

Yes, thats right. This is only the actice pixel array.

> > fps:       15
> > bus-width: 8
> > bpp:       16
> >
> > After re-reading the PIXEL_RATE, maybe I missunderstood the control. As
> > of now I thought it is the amount of bytes per second send on the bus.
> 
> Not bytes but pixels :)
> 
> And the above gives me a 19.660.800 Hz pixel rate

Yep, I re-calced this on my side as well and got the same.

> > But the documentation says pixels per second... Is there a control to
> > expose the current pixelclk on the mbus? What I wanna do in the end is
> > to calculate the througput on the (parallel-)bus.
> 
> Not for parallel busses as far as I'm aware as my understanding is
> that CID_LINK_FREQ applies to CSI-2 setups only.

Yes, while I was working on this topic (4 years back) this was the case.
After your good thoughts I re-checked the control and now it is within
[1] and there the parallel bus is mentioned as well. So this is the
correct control :)

[1] ext-ctrls-image-process.rst

> To get the byte rate on the bus I would simply multiply the pixel
> clock by the number of bytes per pixel, in this case 2.

Please see above. My goal is to request the bus-frequency from the
sensor. With that and the knowledge about the bus-width, we can
calculate the bus throughput.

Regards,
  Marco

> >
> > > > confusion I could rename the mclk_rate to extclk_rate but then clock
> > > > request is not 100% correct since we are requesting a "mclk", this
> > > > should be "extclk".
> > >
> > > Doesn't really make a difference!
> > >
> > > A comment in the code to explain what happens would help though!
> >
> > I did that right now, after your confusion :)
> >
> > Regards,
> >   Marco
> >
> > >
> > > >
> > > > Regards,
> > > >   Marco
> > > >
> > > > > >  	mt9m111->subdev.ctrl_handler = &mt9m111->hdl;
> > > > > >  	if (mt9m111->hdl.error) {
> > > > > >  		ret = mt9m111->hdl.error;
> > > > > > --
> > > > > > 2.30.2
> > > > > >
> > > > >
> > >
> 

Re: [PATCH 2/4] media: mt9m111: fix subdev API usage

[Sakari Ailus]

Hi Marco,

On Thu, Aug 18, 2022 at 04:47:10PM +0200, Marco Felsch wrote:
> In case of I2C transfer failures the driver return failure codes which
> are not allowed according the API documentation. Fix that by ignore the
> register access failure codes.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/media/i2c/mt9m111.c | 116 ++++++++++++++++++++----------------
>  1 file changed, 66 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index cdaf283e1309..53c4dac4e4bd 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -455,7 +455,7 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd,
>  	struct mt9m111 *mt9m111 = to_mt9m111(client);
>  	struct v4l2_rect rect = sel->r;
>  	int width, height;
> -	int ret, align = 0;
> +	int align = 0;
>  
>  	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE ||
>  	    sel->target != V4L2_SEL_TGT_CROP)
> @@ -481,14 +481,13 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd,
>  	width = min(mt9m111->width, rect.width);
>  	height = min(mt9m111->height, rect.height);
>  
> -	ret = mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code);
> -	if (!ret) {
> -		mt9m111->rect = rect;
> -		mt9m111->width = width;
> -		mt9m111->height = height;
> -	}
>  
> -	return ret;
> +	mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code);

If the function can fail, it'd be much better to move it to s_stream
callback than ignore the error.

Same for the rest of such changes.

> +	mt9m111->rect = rect;
> +	mt9m111->width = width;
> +	mt9m111->height = height;
> +
> +	return 0;
>  }
>  
>  static int mt9m111_get_selection(struct v4l2_subdev *sd,
> @@ -558,7 +557,6 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
>  		MT9M111_OUTFMT_RGB444x | MT9M111_OUTFMT_RGBx444 |
>  		MT9M111_OUTFMT_SWAP_YCbCr_C_Y_RGB_EVEN |
>  		MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr_RGB_R_B;
> -	int ret;
>  
>  	switch (code) {
>  	case MEDIA_BUS_FMT_SBGGR8_1X8:
> @@ -613,13 +611,13 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
>  	if (mt9m111->pclk_sample == 0)
>  		mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK;
>  
> -	ret = mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
> -			       data_outfmt2, mask_outfmt2);
> -	if (!ret)
> -		ret = mt9m111_reg_mask(client, context_b.output_fmt_ctrl2,
> -				       data_outfmt2, mask_outfmt2);
>  
> -	return ret;
> +	mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
> +			 data_outfmt2, mask_outfmt2);
> +	mt9m111_reg_mask(client, context_b.output_fmt_ctrl2,
> +			 data_outfmt2, mask_outfmt2);
> +
> +	return 0;
>  }
>  
>  static int mt9m111_set_fmt(struct v4l2_subdev *sd,
> @@ -632,7 +630,6 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
>  	const struct mt9m111_datafmt *fmt;
>  	struct v4l2_rect *rect = &mt9m111->rect;
>  	bool bayer;
> -	int ret;
>  
>  	if (mt9m111->is_streaming)
>  		return -EBUSY;
> @@ -681,16 +678,14 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
>  		return 0;
>  	}
>  
> -	ret = mt9m111_setup_geometry(mt9m111, rect, mf->width, mf->height, mf->code);
> -	if (!ret)
> -		ret = mt9m111_set_pixfmt(mt9m111, mf->code);
> -	if (!ret) {
> -		mt9m111->width	= mf->width;
> -		mt9m111->height	= mf->height;
> -		mt9m111->fmt	= fmt;
> -	}
>  
> -	return ret;
> +	mt9m111_setup_geometry(mt9m111, rect, mf->width, mf->height, mf->code);
> +	mt9m111_set_pixfmt(mt9m111, mf->code);
> +	mt9m111->width	= mf->width;
> +	mt9m111->height	= mf->height;
> +	mt9m111->fmt	= fmt;
> +
> +	return 0;
>  }
>  
>  static const struct mt9m111_mode_info *
> @@ -786,14 +781,13 @@ static int mt9m111_s_register(struct v4l2_subdev *sd,
>  static int mt9m111_set_flip(struct mt9m111 *mt9m111, int flip, int mask)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
> -	int ret;
>  
>  	if (flip)
> -		ret = mt9m111_reg_set(client, mt9m111->ctx->read_mode, mask);
> +		mt9m111_reg_set(client, mt9m111->ctx->read_mode, mask);
>  	else
> -		ret = mt9m111_reg_clear(client, mt9m111->ctx->read_mode, mask);
> +		mt9m111_reg_clear(client, mt9m111->ctx->read_mode, mask);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int mt9m111_get_global_gain(struct mt9m111 *mt9m111)
> @@ -823,7 +817,9 @@ static int mt9m111_set_global_gain(struct mt9m111 *mt9m111, int gain)
>  	else
>  		val = gain;
>  
> -	return reg_write(GLOBAL_GAIN, val);
> +	reg_write(GLOBAL_GAIN, val);
> +
> +	return 0;
>  }
>  
>  static int mt9m111_set_autoexposure(struct mt9m111 *mt9m111, int val)
> @@ -831,8 +827,11 @@ static int mt9m111_set_autoexposure(struct mt9m111 *mt9m111, int val)
>  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
>  
>  	if (val == V4L2_EXPOSURE_AUTO)
> -		return reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
> -	return reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
> +		reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
> +	else
> +		reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
> +
> +	return 0;
>  }
>  
>  static int mt9m111_set_autowhitebalance(struct mt9m111 *mt9m111, int on)
> @@ -840,8 +839,11 @@ static int mt9m111_set_autowhitebalance(struct mt9m111 *mt9m111, int on)
>  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
>  
>  	if (on)
> -		return reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
> -	return reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
> +		reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
> +	else
> +		reg_clear(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
> +
> +	return 0;
>  }
>  
>  static const char * const mt9m111_test_pattern_menu[] = {
> @@ -859,8 +861,9 @@ static int mt9m111_set_test_pattern(struct mt9m111 *mt9m111, int val)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
>  
> -	return mt9m111_reg_mask(client, MT9M111_TPG_CTRL, val,
> -				MT9M111_TPG_SEL_MASK);
> +	mt9m111_reg_mask(client, MT9M111_TPG_CTRL, val, MT9M111_TPG_SEL_MASK);
> +
> +	return 0;
>  }
>  
>  static int mt9m111_set_colorfx(struct mt9m111 *mt9m111, int val)
> @@ -877,9 +880,10 @@ static int mt9m111_set_colorfx(struct mt9m111 *mt9m111, int val)
>  
>  	for (i = 0; i < ARRAY_SIZE(colorfx); i++) {
>  		if (colorfx[i].id == val) {
> -			return mt9m111_reg_mask(client, MT9M111_EFFECTS_MODE,
> -						colorfx[i].value,
> -						MT9M111_EFFECTS_MODE_MASK);
> +			mt9m111_reg_mask(client, MT9M111_EFFECTS_MODE,
> +					 colorfx[i].value,
> +					 MT9M111_EFFECTS_MODE_MASK);
> +			return 0;
>  		}
>  	}
>  
> @@ -890,29 +894,41 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct mt9m111 *mt9m111 = container_of(ctrl->handler,
>  					       struct mt9m111, hdl);
> +	int ret;
>  
>  	switch (ctrl->id) {
>  	case V4L2_CID_VFLIP:
> -		return mt9m111_set_flip(mt9m111, ctrl->val,
> -					MT9M111_RMB_MIRROR_ROWS);
> +		ret = mt9m111_set_flip(mt9m111, ctrl->val,
> +				       MT9M111_RMB_MIRROR_ROWS);
> +		break;
>  	case V4L2_CID_HFLIP:
> -		return mt9m111_set_flip(mt9m111, ctrl->val,
> -					MT9M111_RMB_MIRROR_COLS);
> +		ret = mt9m111_set_flip(mt9m111, ctrl->val,
> +				       MT9M111_RMB_MIRROR_COLS);
> +		break;
>  	case V4L2_CID_GAIN:
> -		return mt9m111_set_global_gain(mt9m111, ctrl->val);
> +		ret = mt9m111_set_global_gain(mt9m111, ctrl->val);
> +		break;
>  	case V4L2_CID_EXPOSURE_AUTO:
> -		return mt9m111_set_autoexposure(mt9m111, ctrl->val);
> +		ret = mt9m111_set_autoexposure(mt9m111, ctrl->val);
> +		break;
>  	case V4L2_CID_AUTO_WHITE_BALANCE:
> -		return mt9m111_set_autowhitebalance(mt9m111, ctrl->val);
> +		ret = mt9m111_set_autowhitebalance(mt9m111, ctrl->val);
> +		break;
>  	case V4L2_CID_TEST_PATTERN:
> -		return mt9m111_set_test_pattern(mt9m111, ctrl->val);
> +		ret = mt9m111_set_test_pattern(mt9m111, ctrl->val);
> +		break;
>  	case V4L2_CID_COLORFX:
> -		return mt9m111_set_colorfx(mt9m111, ctrl->val);
> +		ret = mt9m111_set_colorfx(mt9m111, ctrl->val);
> +		break;
>  	case V4L2_CID_PIXEL_RATE:
> -		return 0;
> +		ret = 0;
> +		break;
> +	default:
> +		ret = -EINVAL;

These were just fine.

>  	}
>  
> -	return -EINVAL;
> +
> +	return ret;
>  }
>  
>  static int mt9m111_suspend(struct mt9m111 *mt9m111)

-- 
Regards,

Sakari Ailus

Re: [PATCH 3/4] media: mt9m111: fix device power usage

[Sakari Ailus]

Hi Marco,

On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote:
> Currently the driver turn off the power after probe and toggle it during
> .stream by using the .s_power callback. This is problematic since other
> callbacks like .set_fmt accessing the hardware as well which will fail.
> So in the end the default format is the only supported format.

It'd be much better to add runtime PM support to the driver instead.

-- 
Sakari Ailus

Re: [PATCH 2/4] media: mt9m111: fix subdev API usage

[Marco Felsch]

Hi Sakari,

On 22-08-22, Sakari Ailus wrote:
> Hi Marco,
> 
> On Thu, Aug 18, 2022 at 04:47:10PM +0200, Marco Felsch wrote:
> > In case of I2C transfer failures the driver return failure codes which
> > are not allowed according the API documentation. Fix that by ignore the
> > register access failure codes.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/media/i2c/mt9m111.c | 116 ++++++++++++++++++++----------------
> >  1 file changed, 66 insertions(+), 50 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > index cdaf283e1309..53c4dac4e4bd 100644
> > --- a/drivers/media/i2c/mt9m111.c
> > +++ b/drivers/media/i2c/mt9m111.c
> > @@ -455,7 +455,7 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd,
> >  	struct mt9m111 *mt9m111 = to_mt9m111(client);
> >  	struct v4l2_rect rect = sel->r;
> >  	int width, height;
> > -	int ret, align = 0;
> > +	int align = 0;
> >  
> >  	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE ||
> >  	    sel->target != V4L2_SEL_TGT_CROP)
> > @@ -481,14 +481,13 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd,
> >  	width = min(mt9m111->width, rect.width);
> >  	height = min(mt9m111->height, rect.height);
> >  
> > -	ret = mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code);
> > -	if (!ret) {
> > -		mt9m111->rect = rect;
> > -		mt9m111->width = width;
> > -		mt9m111->height = height;
> > -	}
> >  
> > -	return ret;
> > +	mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code);
> 
> If the function can fail, it'd be much better to move it to s_stream
> callback than ignore the error.
> 
> Same for the rest of such changes.

I did that in the following patch, but I can merge them if you want.

Regards,
  Marco

Re: [PATCH 3/4] media: mt9m111: fix device power usage

[Marco Felsch]

Hi Sakari,

On 22-08-22, Sakari Ailus wrote:
> Hi Marco,
> 
> On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote:
> > Currently the driver turn off the power after probe and toggle it during
> > .stream by using the .s_power callback. This is problematic since other
> > callbacks like .set_fmt accessing the hardware as well which will fail.
> > So in the end the default format is the only supported format.
> 
> It'd be much better to add runtime PM support to the driver instead.

I got your point, but didn't have the time for it right now, I will drop
the patch from my v2.

Regards,
  Marco

> 
> -- 
> Sakari Ailus
> 

Re: [PATCH 2/4] media: mt9m111: fix subdev API usage

[Sakari Ailus]

On Mon, Aug 22, 2022 at 09:51:01AM +0200, Marco Felsch wrote:
> Hi Sakari,
> 
> On 22-08-22, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Thu, Aug 18, 2022 at 04:47:10PM +0200, Marco Felsch wrote:
> > > In case of I2C transfer failures the driver return failure codes which
> > > are not allowed according the API documentation. Fix that by ignore the
> > > register access failure codes.
> > > 
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > >  drivers/media/i2c/mt9m111.c | 116 ++++++++++++++++++++----------------
> > >  1 file changed, 66 insertions(+), 50 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > > index cdaf283e1309..53c4dac4e4bd 100644
> > > --- a/drivers/media/i2c/mt9m111.c
> > > +++ b/drivers/media/i2c/mt9m111.c
> > > @@ -455,7 +455,7 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd,
> > >  	struct mt9m111 *mt9m111 = to_mt9m111(client);
> > >  	struct v4l2_rect rect = sel->r;
> > >  	int width, height;
> > > -	int ret, align = 0;
> > > +	int align = 0;
> > >  
> > >  	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE ||
> > >  	    sel->target != V4L2_SEL_TGT_CROP)
> > > @@ -481,14 +481,13 @@ static int mt9m111_set_selection(struct v4l2_subdev *sd,
> > >  	width = min(mt9m111->width, rect.width);
> > >  	height = min(mt9m111->height, rect.height);
> > >  
> > > -	ret = mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code);
> > > -	if (!ret) {
> > > -		mt9m111->rect = rect;
> > > -		mt9m111->width = width;
> > > -		mt9m111->height = height;
> > > -	}
> > >  
> > > -	return ret;
> > > +	mt9m111_setup_geometry(mt9m111, &rect, width, height, mt9m111->fmt->code);
> > 
> > If the function can fail, it'd be much better to move it to s_stream
> > callback than ignore the error.
> > 
> > Same for the rest of such changes.
> 
> I did that in the following patch, but I can merge them if you want.

Yes, please.

-- 
Sakari Ailus

Re: [PATCH 3/4] media: mt9m111: fix device power usage

[Sakari Ailus]

On Mon, Aug 22, 2022 at 09:54:26AM +0200, Marco Felsch wrote:
> Hi Sakari,
> 
> On 22-08-22, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote:
> > > Currently the driver turn off the power after probe and toggle it during
> > > .stream by using the .s_power callback. This is problematic since other
> > > callbacks like .set_fmt accessing the hardware as well which will fail.
> > > So in the end the default format is the only supported format.
> > 
> > It'd be much better to add runtime PM support to the driver instead.
> 
> I got your point, but didn't have the time for it right now, I will drop
> the patch from my v2.

The API is different but generally involves doing more or less the same
what this and the 4th patch do together.

-- 
Sakari Ailus

Re: [PATCH 3/4] media: mt9m111: fix device power usage

[Marco Felsch]

Hi Sakari,

On 22-08-22, Sakari Ailus wrote:
> On Mon, Aug 22, 2022 at 09:54:26AM +0200, Marco Felsch wrote:
> > Hi Sakari,
> > 
> > On 22-08-22, Sakari Ailus wrote:
> > > Hi Marco,
> > > 
> > > On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote:
> > > > Currently the driver turn off the power after probe and toggle it during
> > > > .stream by using the .s_power callback. This is problematic since other
> > > > callbacks like .set_fmt accessing the hardware as well which will fail.
> > > > So in the end the default format is the only supported format.
> > > 
> > > It'd be much better to add runtime PM support to the driver instead.
> > 
> > I got your point, but didn't have the time for it right now, I will drop
> > the patch from my v2.
> 
> The API is different but generally involves doing more or less the same
> what this and the 4th patch do together.

I know :) as soon as I got feedback on my TC35 series [1] I give it a
try and change it to dev-pm.

[1] https://lore.kernel.org/linux-media/20220818143307.967150-1-m.felsch@pengutronix.de/T/#t

Regards,
  Marco


> 
> -- 
> Sakari Ailus
> 

Re: [PATCH 3/4] media: mt9m111: fix device power usage

[Sakari Ailus]

Hi Marco,

On Tue, Aug 23, 2022 at 04:44:50PM +0200, Marco Felsch wrote:
> Hi Sakari,
> 
> On 22-08-22, Sakari Ailus wrote:
> > On Mon, Aug 22, 2022 at 09:54:26AM +0200, Marco Felsch wrote:
> > > Hi Sakari,
> > > 
> > > On 22-08-22, Sakari Ailus wrote:
> > > > Hi Marco,
> > > > 
> > > > On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote:
> > > > > Currently the driver turn off the power after probe and toggle it during
> > > > > .stream by using the .s_power callback. This is problematic since other
> > > > > callbacks like .set_fmt accessing the hardware as well which will fail.
> > > > > So in the end the default format is the only supported format.
> > > > 
> > > > It'd be much better to add runtime PM support to the driver instead.
> > > 
> > > I got your point, but didn't have the time for it right now, I will drop
> > > the patch from my v2.
> > 
> > The API is different but generally involves doing more or less the same
> > what this and the 4th patch do together.
> 
> I know :) as soon as I got feedback on my TC35 series [1] I give it a
> try and change it to dev-pm.

What's the status of this set?

These are nice improvements but I was expecting v2.

Thanks.

-- 
Sakari Ailus

Re: [PATCH 3/4] media: mt9m111: fix device power usage

[Marco Felsch]

Hi Sakari,

On 23-01-17, Sakari Ailus wrote:
> Hi Marco,
> 
> On Tue, Aug 23, 2022 at 04:44:50PM +0200, Marco Felsch wrote:
> > Hi Sakari,
> > 
> > On 22-08-22, Sakari Ailus wrote:
> > > On Mon, Aug 22, 2022 at 09:54:26AM +0200, Marco Felsch wrote:
> > > > Hi Sakari,
> > > > 
> > > > On 22-08-22, Sakari Ailus wrote:
> > > > > Hi Marco,
> > > > > 
> > > > > On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote:
> > > > > > Currently the driver turn off the power after probe and toggle it during
> > > > > > .stream by using the .s_power callback. This is problematic since other
> > > > > > callbacks like .set_fmt accessing the hardware as well which will fail.
> > > > > > So in the end the default format is the only supported format.
> > > > > 
> > > > > It'd be much better to add runtime PM support to the driver instead.
> > > > 
> > > > I got your point, but didn't have the time for it right now, I will drop
> > > > the patch from my v2.
> > > 
> > > The API is different but generally involves doing more or less the same
> > > what this and the 4th patch do together.
> > 
> > I know :) as soon as I got feedback on my TC35 series [1] I give it a
> > try and change it to dev-pm.
> 
> What's the status of this set?
> 
> These are nice improvements but I was expecting v2.

Unfortunately this was just a testing vehicle I had for the TC35 bridge
chip. As soon as I have access to it again I will send a new version.

Regards,
  Marco

Re: [PATCH 3/4] media: mt9m111: fix device power usage

[Sakari Ailus]

On Tue, Jan 17, 2023 at 12:29:59PM +0100, Marco Felsch wrote:
> Hi Sakari,
> 
> On 23-01-17, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Tue, Aug 23, 2022 at 04:44:50PM +0200, Marco Felsch wrote:
> > > Hi Sakari,
> > > 
> > > On 22-08-22, Sakari Ailus wrote:
> > > > On Mon, Aug 22, 2022 at 09:54:26AM +0200, Marco Felsch wrote:
> > > > > Hi Sakari,
> > > > > 
> > > > > On 22-08-22, Sakari Ailus wrote:
> > > > > > Hi Marco,
> > > > > > 
> > > > > > On Thu, Aug 18, 2022 at 04:47:11PM +0200, Marco Felsch wrote:
> > > > > > > Currently the driver turn off the power after probe and toggle it during
> > > > > > > .stream by using the .s_power callback. This is problematic since other
> > > > > > > callbacks like .set_fmt accessing the hardware as well which will fail.
> > > > > > > So in the end the default format is the only supported format.
> > > > > > 
> > > > > > It'd be much better to add runtime PM support to the driver instead.
> > > > > 
> > > > > I got your point, but didn't have the time for it right now, I will drop
> > > > > the patch from my v2.
> > > > 
> > > > The API is different but generally involves doing more or less the same
> > > > what this and the 4th patch do together.
> > > 
> > > I know :) as soon as I got feedback on my TC35 series [1] I give it a
> > > try and change it to dev-pm.
> > 
> > What's the status of this set?
> > 
> > These are nice improvements but I was expecting v2.
> 
> Unfortunately this was just a testing vehicle I had for the TC35 bridge
> chip. As soon as I have access to it again I will send a new version.

Ack, thanks!

-- 
Sakari Ailus