[PATCH v2 1/3] media: mt9m111: add V4L2_CID_LINK_FREQ support

[PATCH v2 1/3] media: mt9m111: add V4L2_CID_LINK_FREQ support

[Marco Felsch]

Add support to report the link frequency.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
The v1 of this small series can be found here:
https://lore.kernel.org/all/20220818144712.997477-1-m.felsch@pengutronix.de/

Thanks a lot to Jacopo for the review feedback on my v1.

Changelog:

v2:
- use V4L2_CID_LINK_FREQ instead of V4L2_CID_PIXEL_RATE
---
 drivers/media/i2c/mt9m111.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index afc86efa9e3e..52be1c310455 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -1249,6 +1249,8 @@ static int mt9m111_probe(struct i2c_client *client)
 {
 	struct mt9m111 *mt9m111;
 	struct i2c_adapter *adapter = client->adapter;
+	static s64 extclk_rate;
+	struct v4l2_ctrl *ctrl;
 	int ret;
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
@@ -1271,6 +1273,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;
+
+	extclk_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 +1294,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 +1318,16 @@ static int mt9m111_probe(struct i2c_client *client)
 				BIT(V4L2_COLORFX_NEGATIVE) |
 				BIT(V4L2_COLORFX_SOLARIZATION)),
 			V4L2_COLORFX_NONE);
+	/*
+	 * The extclk rate equals the link freq. if reg default values are used,
+	 * which is the case. This must be adapted as soon as we don't use the
+	 * default values anymore.
+	 */
+	ctrl = v4l2_ctrl_new_int_menu(&mt9m111->hdl, &mt9m111_ctrl_ops,
+				      V4L2_CID_LINK_FREQ, 0, 0, &extclk_rate);
+	if (ctrl)
+		ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
 	mt9m111->subdev.ctrl_handler = &mt9m111->hdl;
 	if (mt9m111->hdl.error) {
 		ret = mt9m111->hdl.error;
-- 
2.30.2

[PATCH v2 2/3] 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 for
.set_fmt() and .set_selection(). For the debug register access we need
to turn the device on/off before accessing the registers to fix this and
finally for the ctrls access we need the device to be powered to fix
this.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
Changelog:

v2:
- squash patch 2 and 3
---
 drivers/media/i2c/mt9m111.c | 40 ++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 52be1c310455..8de93ab99cbc 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,11 @@ 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;
-	}
+	mt9m111->rect = rect;
+	mt9m111->width = width;
+	mt9m111->height = height;
 
-	return ret;
+	return 0;
 }
 
 static int mt9m111_get_selection(struct v4l2_subdev *sd,
@@ -632,7 +629,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 +677,11 @@ 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;
-	}
+	mt9m111->width	= mf->width;
+	mt9m111->height	= mf->height;
+	mt9m111->fmt	= fmt;
 
-	return ret;
+	return 0;
 }
 
 static const struct mt9m111_mode_info *
@@ -748,6 +739,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)
@@ -758,10 +751,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;
 
@@ -776,9 +773,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
@@ -891,6 +892,9 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
 	struct mt9m111 *mt9m111 = container_of(ctrl->handler,
 					       struct mt9m111, hdl);
 
+	if (!mt9m111->is_streaming)
+		return 0;
+
 	switch (ctrl->id) {
 	case V4L2_CID_VFLIP:
 		return mt9m111_set_flip(mt9m111, ctrl->val,
-- 
2.30.2

[PATCH v2 3/3] media: mt9m111: don't turn on the output while powering it

[Marco Felsch]

Currently the .s_power() turn on/off the device and enables/disables the
sensor output. This is wrong since it should only handle the power not
not the sensor output behaviour. Enabling the sensor output should be
part of the .s_stream() callback.

Fix this by adding mt9m111_set_output() which gets called by the
.s_stream() callback and remove the output register bits from
mt9m111_resume/suspend.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
Changelog:

v2:
- new patch, replaces: "media: mt9m111: remove .s_power callback"
---
 drivers/media/i2c/mt9m111.c | 38 ++++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 8de93ab99cbc..2cc0b0da7636 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -426,10 +426,25 @@ static int mt9m111_setup_geometry(struct mt9m111 *mt9m111, struct v4l2_rect *rec
 	return ret;
 }
 
-static int mt9m111_enable(struct mt9m111 *mt9m111)
+static int mt9m111_set_output(struct mt9m111 *mt9m111, int on)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
-	return reg_write(RESET, MT9M111_RESET_CHIP_ENABLE);
+	int ret;
+
+	if (on) {
+		ret = reg_clear(RESET, MT9M111_RESET_OUTPUT_DISABLE);
+		if (ret)
+			return ret;
+
+		return reg_set(RESET, MT9M111_RESET_CHIP_ENABLE);
+	}
+
+	/* disable */
+	ret = reg_set(RESET, MT9M111_RESET_OUTPUT_DISABLE);
+	if (ret)
+		return ret;
+
+	return reg_clear(RESET, MT9M111_RESET_CHIP_ENABLE);
 }
 
 static int mt9m111_reset(struct mt9m111 *mt9m111)
@@ -927,10 +942,7 @@ static int mt9m111_suspend(struct mt9m111 *mt9m111)
 	ret = reg_set(RESET, MT9M111_RESET_RESET_MODE);
 	if (!ret)
 		ret = reg_set(RESET, MT9M111_RESET_RESET_SOC |
-			      MT9M111_RESET_OUTPUT_DISABLE |
 			      MT9M111_RESET_ANALOG_STANDBY);
-	if (!ret)
-		ret = reg_clear(RESET, MT9M111_RESET_CHIP_ENABLE);
 
 	return ret;
 }
@@ -951,9 +963,9 @@ static void mt9m111_restore_state(struct mt9m111 *mt9m111)
 
 static int mt9m111_resume(struct mt9m111 *mt9m111)
 {
-	int ret = mt9m111_enable(mt9m111);
-	if (!ret)
-		ret = mt9m111_reset(mt9m111);
+	int ret;
+
+	ret = mt9m111_reset(mt9m111);
 	if (!ret)
 		mt9m111_restore_state(mt9m111);
 
@@ -965,9 +977,7 @@ static int mt9m111_init(struct mt9m111 *mt9m111)
 	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
 	int ret;
 
-	ret = mt9m111_enable(mt9m111);
-	if (!ret)
-		ret = mt9m111_reset(mt9m111);
+	ret = mt9m111_reset(mt9m111);
 	if (!ret)
 		ret = mt9m111_set_context(mt9m111, mt9m111->ctx);
 	if (ret)
@@ -1116,8 +1126,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;
+
+	ret = mt9m111_set_output(mt9m111, enable);
+	if (ret)
+		return ret;
 
 	mt9m111->is_streaming = !!enable;
+
 	return 0;
 }
 
-- 
2.30.2

Re: [PATCH v2 1/3] media: mt9m111: add V4L2_CID_LINK_FREQ support

[Sakari Ailus]

Hi Marco,

On Fri, Sep 16, 2022 at 03:57:11PM +0200, Marco Felsch wrote:
> Add support to report the link frequency.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> The v1 of this small series can be found here:
> https://lore.kernel.org/all/20220818144712.997477-1-m.felsch@pengutronix.de/
> 
> Thanks a lot to Jacopo for the review feedback on my v1.
> 
> Changelog:
> 
> v2:
> - use V4L2_CID_LINK_FREQ instead of V4L2_CID_PIXEL_RATE
> ---
>  drivers/media/i2c/mt9m111.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index afc86efa9e3e..52be1c310455 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -1249,6 +1249,8 @@ static int mt9m111_probe(struct i2c_client *client)
>  {
>  	struct mt9m111 *mt9m111;
>  	struct i2c_adapter *adapter = client->adapter;
> +	static s64 extclk_rate;
> +	struct v4l2_ctrl *ctrl;
>  	int ret;
>  
>  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
> @@ -1271,6 +1273,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;
> +
> +	extclk_rate = clk_get_rate(mt9m111->clk);
> +	clk_disable_unprepare(mt9m111->clk);

I don't think you'll need to enable a clock to just get its frequency.

> +
>  	mt9m111->regulator = devm_regulator_get(&client->dev, "vdd");
>  	if (IS_ERR(mt9m111->regulator)) {
>  		dev_err(&client->dev, "regulator not found: %ld\n",
> @@ -1285,7 +1294,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 +1318,16 @@ static int mt9m111_probe(struct i2c_client *client)
>  				BIT(V4L2_COLORFX_NEGATIVE) |
>  				BIT(V4L2_COLORFX_SOLARIZATION)),
>  			V4L2_COLORFX_NONE);
> +	/*
> +	 * The extclk rate equals the link freq. if reg default values are used,
> +	 * which is the case. This must be adapted as soon as we don't use the
> +	 * default values anymore.
> +	 */
> +	ctrl = v4l2_ctrl_new_int_menu(&mt9m111->hdl, &mt9m111_ctrl_ops,
> +				      V4L2_CID_LINK_FREQ, 0, 0, &extclk_rate);
> +	if (ctrl)
> +		ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
>  	mt9m111->subdev.ctrl_handler = &mt9m111->hdl;
>  	if (mt9m111->hdl.error) {
>  		ret = mt9m111->hdl.error;

-- 
Regards,

Sakari Ailus

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

[Sakari Ailus]

Hi Marco,

On Fri, Sep 16, 2022 at 03:57:12PM +0200, Marco Felsch wrote:
> @@ -758,10 +751,14 @@ static int mt9m111_g_register(struct v4l2_subdev *sd,
>  	if (reg->reg > 0x2ff)
>  		return -EINVAL;
>  
> +	mt9m111_s_power(sd, 1);

It would be nice to have this driver converted to runtime PM. Up to you.

-- 
Sakari Ailus

Re: [PATCH v2 1/3] media: mt9m111: add V4L2_CID_LINK_FREQ support

[Laurent Pinchart]

On Mon, Sep 19, 2022 at 12:42:15PM +0000, Sakari Ailus wrote:
> Hi Marco,
> 
> On Fri, Sep 16, 2022 at 03:57:11PM +0200, Marco Felsch wrote:
> > Add support to report the link frequency.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > The v1 of this small series can be found here:
> > https://lore.kernel.org/all/20220818144712.997477-1-m.felsch@pengutronix.de/
> > 
> > Thanks a lot to Jacopo for the review feedback on my v1.
> > 
> > Changelog:
> > 
> > v2:
> > - use V4L2_CID_LINK_FREQ instead of V4L2_CID_PIXEL_RATE
> > ---
> >  drivers/media/i2c/mt9m111.c | 21 ++++++++++++++++++++-
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > index afc86efa9e3e..52be1c310455 100644
> > --- a/drivers/media/i2c/mt9m111.c
> > +++ b/drivers/media/i2c/mt9m111.c
> > @@ -1249,6 +1249,8 @@ static int mt9m111_probe(struct i2c_client *client)
> >  {
> >  	struct mt9m111 *mt9m111;
> >  	struct i2c_adapter *adapter = client->adapter;
> > +	static s64 extclk_rate;
> > +	struct v4l2_ctrl *ctrl;
> >  	int ret;
> >  
> >  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
> > @@ -1271,6 +1273,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;
> > +
> > +	extclk_rate = clk_get_rate(mt9m111->clk);
> > +	clk_disable_unprepare(mt9m111->clk);
> 
> I don't think you'll need to enable a clock to just get its frequency.
> 
> > +
> >  	mt9m111->regulator = devm_regulator_get(&client->dev, "vdd");
> >  	if (IS_ERR(mt9m111->regulator)) {
> >  		dev_err(&client->dev, "regulator not found: %ld\n",
> > @@ -1285,7 +1294,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 +1318,16 @@ static int mt9m111_probe(struct i2c_client *client)
> >  				BIT(V4L2_COLORFX_NEGATIVE) |
> >  				BIT(V4L2_COLORFX_SOLARIZATION)),
> >  			V4L2_COLORFX_NONE);
> > +	/*
> > +	 * The extclk rate equals the link freq. if reg default values are used,

s/freq./frequency/

> > +	 * which is the case. This must be adapted as soon as we don't use the
> > +	 * default values anymore.
> > +	 */
> > +	ctrl = v4l2_ctrl_new_int_menu(&mt9m111->hdl, &mt9m111_ctrl_ops,
> > +				      V4L2_CID_LINK_FREQ, 0, 0, &extclk_rate);
> > +	if (ctrl)
> > +		ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> >  	mt9m111->subdev.ctrl_handler = &mt9m111->hdl;
> >  	if (mt9m111->hdl.error) {
> >  		ret = mt9m111->hdl.error;

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

On Mon, Sep 19, 2022 at 12:45:42PM +0000, Sakari Ailus wrote:
> Hi Marco,
> 
> On Fri, Sep 16, 2022 at 03:57:12PM +0200, Marco Felsch wrote:
> > @@ -758,10 +751,14 @@ static int mt9m111_g_register(struct v4l2_subdev *sd,
> >  	if (reg->reg > 0x2ff)
> >  		return -EINVAL;
> >  
> > +	mt9m111_s_power(sd, 1);
> 
> It would be nice to have this driver converted to runtime PM. Up to you.

I second that :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/3] media: mt9m111: don't turn on the output while powering it

[Laurent Pinchart]

Hi Marco,

Thank you for the patch.

On Fri, Sep 16, 2022 at 03:57:13PM +0200, Marco Felsch wrote:
> Currently the .s_power() turn on/off the device and enables/disables the
> sensor output. This is wrong since it should only handle the power not
> not the sensor output behaviour. Enabling the sensor output should be
> part of the .s_stream() callback.
> 
> Fix this by adding mt9m111_set_output() which gets called by the
> .s_stream() callback and remove the output register bits from
> mt9m111_resume/suspend.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> Changelog:
> 
> v2:
> - new patch, replaces: "media: mt9m111: remove .s_power callback"
> ---
>  drivers/media/i2c/mt9m111.c | 38 ++++++++++++++++++++++++++-----------
>  1 file changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index 8de93ab99cbc..2cc0b0da7636 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -426,10 +426,25 @@ static int mt9m111_setup_geometry(struct mt9m111 *mt9m111, struct v4l2_rect *rec
>  	return ret;
>  }
>  
> -static int mt9m111_enable(struct mt9m111 *mt9m111)
> +static int mt9m111_set_output(struct mt9m111 *mt9m111, int on)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
> -	return reg_write(RESET, MT9M111_RESET_CHIP_ENABLE);
> +	int ret;
> +
> +	if (on) {
> +		ret = reg_clear(RESET, MT9M111_RESET_OUTPUT_DISABLE);
> +		if (ret)
> +			return ret;
> +
> +		return reg_set(RESET, MT9M111_RESET_CHIP_ENABLE);
> +	}
> +
> +	/* disable */
> +	ret = reg_set(RESET, MT9M111_RESET_OUTPUT_DISABLE);
> +	if (ret)
> +		return ret;
> +
> +	return reg_clear(RESET, MT9M111_RESET_CHIP_ENABLE);

Unless the hardware specifically requires this sequence, I'd use the
inverse of the enable sequence here.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  }
>  
>  static int mt9m111_reset(struct mt9m111 *mt9m111)
> @@ -927,10 +942,7 @@ static int mt9m111_suspend(struct mt9m111 *mt9m111)
>  	ret = reg_set(RESET, MT9M111_RESET_RESET_MODE);
>  	if (!ret)
>  		ret = reg_set(RESET, MT9M111_RESET_RESET_SOC |
> -			      MT9M111_RESET_OUTPUT_DISABLE |
>  			      MT9M111_RESET_ANALOG_STANDBY);
> -	if (!ret)
> -		ret = reg_clear(RESET, MT9M111_RESET_CHIP_ENABLE);
>  
>  	return ret;
>  }
> @@ -951,9 +963,9 @@ static void mt9m111_restore_state(struct mt9m111 *mt9m111)
>  
>  static int mt9m111_resume(struct mt9m111 *mt9m111)
>  {
> -	int ret = mt9m111_enable(mt9m111);
> -	if (!ret)
> -		ret = mt9m111_reset(mt9m111);
> +	int ret;
> +
> +	ret = mt9m111_reset(mt9m111);
>  	if (!ret)
>  		mt9m111_restore_state(mt9m111);
>  
> @@ -965,9 +977,7 @@ static int mt9m111_init(struct mt9m111 *mt9m111)
>  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
>  	int ret;
>  
> -	ret = mt9m111_enable(mt9m111);
> -	if (!ret)
> -		ret = mt9m111_reset(mt9m111);
> +	ret = mt9m111_reset(mt9m111);
>  	if (!ret)
>  		ret = mt9m111_set_context(mt9m111, mt9m111->ctx);
>  	if (ret)
> @@ -1116,8 +1126,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;
> +
> +	ret = mt9m111_set_output(mt9m111, enable);
> +	if (ret)
> +		return ret;
>  
>  	mt9m111->is_streaming = !!enable;
> +
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/3] media: mt9m111: add V4L2_CID_LINK_FREQ support

[Marco Felsch]

Hi Sakari,

On 22-09-19, Sakari Ailus wrote:
> Hi Marco,
> 
> On Fri, Sep 16, 2022 at 03:57:11PM +0200, Marco Felsch wrote:
> > Add support to report the link frequency.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > The v1 of this small series can be found here:
> > https://lore.kernel.org/all/20220818144712.997477-1-m.felsch@pengutronix.de/
> > 
> > Thanks a lot to Jacopo for the review feedback on my v1.
> > 
> > Changelog:
> > 
> > v2:
> > - use V4L2_CID_LINK_FREQ instead of V4L2_CID_PIXEL_RATE
> > ---
> >  drivers/media/i2c/mt9m111.c | 21 ++++++++++++++++++++-
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > index afc86efa9e3e..52be1c310455 100644
> > --- a/drivers/media/i2c/mt9m111.c
> > +++ b/drivers/media/i2c/mt9m111.c
> > @@ -1249,6 +1249,8 @@ static int mt9m111_probe(struct i2c_client *client)
> >  {
> >  	struct mt9m111 *mt9m111;
> >  	struct i2c_adapter *adapter = client->adapter;
> > +	static s64 extclk_rate;
> > +	struct v4l2_ctrl *ctrl;
> >  	int ret;
> >  
> >  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
> > @@ -1271,6 +1273,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;
> > +
> > +	extclk_rate = clk_get_rate(mt9m111->clk);
> > +	clk_disable_unprepare(mt9m111->clk);
> 
> I don't think you'll need to enable a clock to just get its frequency.

The official API states that you need to turn on the clk before
requesting it and it makes sense. Also there is a new helper
devm_clk_get_enabled() which addresses simple clk usage since most of
drivers don't enable it before requesting the rate.

Regards,
  Marco

> > +
> >  	mt9m111->regulator = devm_regulator_get(&client->dev, "vdd");
> >  	if (IS_ERR(mt9m111->regulator)) {
> >  		dev_err(&client->dev, "regulator not found: %ld\n",
> > @@ -1285,7 +1294,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 +1318,16 @@ static int mt9m111_probe(struct i2c_client *client)
> >  				BIT(V4L2_COLORFX_NEGATIVE) |
> >  				BIT(V4L2_COLORFX_SOLARIZATION)),
> >  			V4L2_COLORFX_NONE);
> > +	/*
> > +	 * The extclk rate equals the link freq. if reg default values are used,
> > +	 * which is the case. This must be adapted as soon as we don't use the
> > +	 * default values anymore.
> > +	 */
> > +	ctrl = v4l2_ctrl_new_int_menu(&mt9m111->hdl, &mt9m111_ctrl_ops,
> > +				      V4L2_CID_LINK_FREQ, 0, 0, &extclk_rate);
> > +	if (ctrl)
> > +		ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> >  	mt9m111->subdev.ctrl_handler = &mt9m111->hdl;
> >  	if (mt9m111->hdl.error) {
> >  		ret = mt9m111->hdl.error;
> 
> -- 
> Regards,
> 
> Sakari Ailus
> 

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

[Marco Felsch]

On 22-09-19, Laurent Pinchart wrote:
> On Mon, Sep 19, 2022 at 12:45:42PM +0000, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Fri, Sep 16, 2022 at 03:57:12PM +0200, Marco Felsch wrote:
> > > @@ -758,10 +751,14 @@ static int mt9m111_g_register(struct v4l2_subdev *sd,
> > >  	if (reg->reg > 0x2ff)
> > >  		return -EINVAL;
> > >  
> > > +	mt9m111_s_power(sd, 1);
> > 
> > It would be nice to have this driver converted to runtime PM. Up to you.
> 
> I second that :-)

I would rather keep it this way and add 2nd patch to change. So it would
be easier to backport the patch.

Regards,
  Marco

> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

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

[Sakari Ailus]

On Mon, Sep 19, 2022 at 03:10:09PM +0200, Marco Felsch wrote:
> On 22-09-19, Laurent Pinchart wrote:
> > On Mon, Sep 19, 2022 at 12:45:42PM +0000, Sakari Ailus wrote:
> > > Hi Marco,
> > > 
> > > On Fri, Sep 16, 2022 at 03:57:12PM +0200, Marco Felsch wrote:
> > > > @@ -758,10 +751,14 @@ static int mt9m111_g_register(struct v4l2_subdev *sd,
> > > >  	if (reg->reg > 0x2ff)
> > > >  		return -EINVAL;
> > > >  
> > > > +	mt9m111_s_power(sd, 1);
> > > 
> > > It would be nice to have this driver converted to runtime PM. Up to you.
> > 
> > I second that :-)
> 
> I would rather keep it this way and add 2nd patch to change. So it would
> be easier to backport the patch.

Works for me.

-- 
Sakari Ailus

Re: [PATCH v2 3/3] media: mt9m111: don't turn on the output while powering it

[Marco Felsch]

Hi Laurent,

On 22-09-19, Laurent Pinchart wrote:
> Hi Marco,
> 
> Thank you for the patch.
> 
> On Fri, Sep 16, 2022 at 03:57:13PM +0200, Marco Felsch wrote:
> > Currently the .s_power() turn on/off the device and enables/disables the
> > sensor output. This is wrong since it should only handle the power not
> > not the sensor output behaviour. Enabling the sensor output should be
> > part of the .s_stream() callback.
> > 
> > Fix this by adding mt9m111_set_output() which gets called by the
> > .s_stream() callback and remove the output register bits from
> > mt9m111_resume/suspend.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > Changelog:
> > 
> > v2:
> > - new patch, replaces: "media: mt9m111: remove .s_power callback"
> > ---
> >  drivers/media/i2c/mt9m111.c | 38 ++++++++++++++++++++++++++-----------
> >  1 file changed, 27 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > index 8de93ab99cbc..2cc0b0da7636 100644
> > --- a/drivers/media/i2c/mt9m111.c
> > +++ b/drivers/media/i2c/mt9m111.c
> > @@ -426,10 +426,25 @@ static int mt9m111_setup_geometry(struct mt9m111 *mt9m111, struct v4l2_rect *rec
> >  	return ret;
> >  }
> >  
> > -static int mt9m111_enable(struct mt9m111 *mt9m111)
> > +static int mt9m111_set_output(struct mt9m111 *mt9m111, int on)
> >  {
> >  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
> > -	return reg_write(RESET, MT9M111_RESET_CHIP_ENABLE);
> > +	int ret;
> > +
> > +	if (on) {
> > +		ret = reg_clear(RESET, MT9M111_RESET_OUTPUT_DISABLE);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return reg_set(RESET, MT9M111_RESET_CHIP_ENABLE);
> > +	}
> > +
> > +	/* disable */
> > +	ret = reg_set(RESET, MT9M111_RESET_OUTPUT_DISABLE);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return reg_clear(RESET, MT9M111_RESET_CHIP_ENABLE);
> 
> Unless the hardware specifically requires this sequence, I'd use the
> inverse of the enable sequence here.

Output-disable: -> put output pins into tri-state.
chip-enable:  -> reset sensor readout path.

Enable:
  -> set output pins accordingly
     -> put sensor out of reset --> start sensor readout

Disable:
  -> set output pins to tri-state
     -> put sensor into reset --> stop sensor readout

To avoid possible artifacts I disabled the pins first before resetting
the sensor core (stopping the readout).

Regards,
  Marco

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> >  }
> >  
> >  static int mt9m111_reset(struct mt9m111 *mt9m111)
> > @@ -927,10 +942,7 @@ static int mt9m111_suspend(struct mt9m111 *mt9m111)
> >  	ret = reg_set(RESET, MT9M111_RESET_RESET_MODE);
> >  	if (!ret)
> >  		ret = reg_set(RESET, MT9M111_RESET_RESET_SOC |
> > -			      MT9M111_RESET_OUTPUT_DISABLE |
> >  			      MT9M111_RESET_ANALOG_STANDBY);
> > -	if (!ret)
> > -		ret = reg_clear(RESET, MT9M111_RESET_CHIP_ENABLE);
> >  
> >  	return ret;
> >  }
> > @@ -951,9 +963,9 @@ static void mt9m111_restore_state(struct mt9m111 *mt9m111)
> >  
> >  static int mt9m111_resume(struct mt9m111 *mt9m111)
> >  {
> > -	int ret = mt9m111_enable(mt9m111);
> > -	if (!ret)
> > -		ret = mt9m111_reset(mt9m111);
> > +	int ret;
> > +
> > +	ret = mt9m111_reset(mt9m111);
> >  	if (!ret)
> >  		mt9m111_restore_state(mt9m111);
> >  
> > @@ -965,9 +977,7 @@ static int mt9m111_init(struct mt9m111 *mt9m111)
> >  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
> >  	int ret;
> >  
> > -	ret = mt9m111_enable(mt9m111);
> > -	if (!ret)
> > -		ret = mt9m111_reset(mt9m111);
> > +	ret = mt9m111_reset(mt9m111);
> >  	if (!ret)
> >  		ret = mt9m111_set_context(mt9m111, mt9m111->ctx);
> >  	if (ret)
> > @@ -1116,8 +1126,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;
> > +
> > +	ret = mt9m111_set_output(mt9m111, enable);
> > +	if (ret)
> > +		return ret;
> >  
> >  	mt9m111->is_streaming = !!enable;
> > +
> >  	return 0;
> >  }
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

Re: [PATCH v2 1/3] media: mt9m111: add V4L2_CID_LINK_FREQ support

[Sakari Ailus]

Hi Marco,

On Mon, Sep 19, 2022 at 03:08:29PM +0200, Marco Felsch wrote:
> Hi Sakari,
> 
> On 22-09-19, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Fri, Sep 16, 2022 at 03:57:11PM +0200, Marco Felsch wrote:
> > > Add support to report the link frequency.
> > > 
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > > The v1 of this small series can be found here:
> > > https://lore.kernel.org/all/20220818144712.997477-1-m.felsch@pengutronix.de/
> > > 
> > > Thanks a lot to Jacopo for the review feedback on my v1.
> > > 
> > > Changelog:
> > > 
> > > v2:
> > > - use V4L2_CID_LINK_FREQ instead of V4L2_CID_PIXEL_RATE
> > > ---
> > >  drivers/media/i2c/mt9m111.c | 21 ++++++++++++++++++++-
> > >  1 file changed, 20 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > > index afc86efa9e3e..52be1c310455 100644
> > > --- a/drivers/media/i2c/mt9m111.c
> > > +++ b/drivers/media/i2c/mt9m111.c
> > > @@ -1249,6 +1249,8 @@ static int mt9m111_probe(struct i2c_client *client)
> > >  {
> > >  	struct mt9m111 *mt9m111;
> > >  	struct i2c_adapter *adapter = client->adapter;
> > > +	static s64 extclk_rate;
> > > +	struct v4l2_ctrl *ctrl;
> > >  	int ret;
> > >  
> > >  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
> > > @@ -1271,6 +1273,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;
> > > +
> > > +	extclk_rate = clk_get_rate(mt9m111->clk);
> > > +	clk_disable_unprepare(mt9m111->clk);
> > 
> > I don't think you'll need to enable a clock to just get its frequency.
> 
> The official API states that you need to turn on the clk before
> requesting it and it makes sense. Also there is a new helper
> devm_clk_get_enabled() which addresses simple clk usage since most of
> drivers don't enable it before requesting the rate.

I guess the rate could change in the meantime, unless exclusive access is
requested. The clock framework currently doesn't offer a way to set the
assigned rate and prevent changing it. But above, couldn't the clock
frequency be changed again once the clock has been disabled?

-- 
Sakari Ailus

Re: [PATCH v2 1/3] media: mt9m111: add V4L2_CID_LINK_FREQ support

[Marco Felsch]

Hi Sakari,

On 22-09-19, Sakari Ailus wrote:

...

> > > > +	ret = clk_prepare_enable(mt9m111->clk);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	extclk_rate = clk_get_rate(mt9m111->clk);
> > > > +	clk_disable_unprepare(mt9m111->clk);
> > > 
> > > I don't think you'll need to enable a clock to just get its frequency.
> > 
> > The official API states that you need to turn on the clk before
> > requesting it and it makes sense. Also there is a new helper
> > devm_clk_get_enabled() which addresses simple clk usage since most of
> > drivers don't enable it before requesting the rate.
> 
> I guess the rate could change in the meantime, unless exclusive access is
> requested. 

Not only that, there are a bunch of clk provider hw around which may
need to turned on first. Anyway, I really don't care on this topic. As
I said I wanted to fullfil the API and if drop clk_prepare_enable() I
don't. So if this okay for you I will go that way.

> The clock framework currently doesn't offer a way to set the assigned
> rate and prevent changing it. But above, couldn't the clock frequency
> be changed again once the clock has been disabled?

Yes it could.

Regards,
  Marco

Re: [PATCH v2 1/3] media: mt9m111: add V4L2_CID_LINK_FREQ support

[Jacopo Mondi]

Hello

On Tue, Sep 20, 2022 at 10:56:17AM +0200, Marco Felsch wrote:
> Hi Sakari,
>
> On 22-09-19, Sakari Ailus wrote:
>
> ...
>
> > > > > +	ret = clk_prepare_enable(mt9m111->clk);
> > > > > +	if (ret < 0)
> > > > > +		return ret;
> > > > > +
> > > > > +	extclk_rate = clk_get_rate(mt9m111->clk);
> > > > > +	clk_disable_unprepare(mt9m111->clk);
> > > >
> > > > I don't think you'll need to enable a clock to just get its frequency.
> > >
> > > The official API states that you need to turn on the clk before
> > > requesting it and it makes sense. Also there is a new helper
> > > devm_clk_get_enabled() which addresses simple clk usage since most of
> > > drivers don't enable it before requesting the rate.

Had the same question on v1 and Marco pointed me to the clk_get_rate()
documentation
https://elixir.bootlin.com/linux/v6.0-rc1/source/include/linux/clk.h#L682

which indeed specifies
"This is only valid once the clock source has been enabled."

However none (or very few) of the linux-media i2c drivers actually do
that.

I have added in cc the clk framework maintainer to see if he can help
shed some light on this


> >
> > I guess the rate could change in the meantime, unless exclusive access is
> > requested.
>
> Not only that, there are a bunch of clk provider hw around which may
> need to turned on first. Anyway, I really don't care on this topic. As
> I said I wanted to fullfil the API and if drop clk_prepare_enable() I
> don't. So if this okay for you I will go that way.
>
> > The clock framework currently doesn't offer a way to set the assigned
> > rate and prevent changing it. But above, couldn't the clock frequency
> > be changed again once the clock has been disabled?
>
> Yes it could.
>
> Regards,
>   Marco

Re: [PATCH v2 1/3] media: mt9m111: add V4L2_CID_LINK_FREQ support

[Jacopo Mondi]

Hi Marco

On Fri, Sep 16, 2022 at 03:57:11PM +0200, Marco Felsch wrote:
> Add support to report the link frequency.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> The v1 of this small series can be found here:
> https://lore.kernel.org/all/20220818144712.997477-1-m.felsch@pengutronix.de/
>
> Thanks a lot to Jacopo for the review feedback on my v1.
>
> Changelog:
>
> v2:
> - use V4L2_CID_LINK_FREQ instead of V4L2_CID_PIXEL_RATE
> ---
>  drivers/media/i2c/mt9m111.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index afc86efa9e3e..52be1c310455 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -1249,6 +1249,8 @@ static int mt9m111_probe(struct i2c_client *client)
>  {
>  	struct mt9m111 *mt9m111;
>  	struct i2c_adapter *adapter = client->adapter;
> +	static s64 extclk_rate;

Why static ?
Also clk_get_rate() returns an unsigned long


> +	struct v4l2_ctrl *ctrl;
>  	int ret;
>
>  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
> @@ -1271,6 +1273,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;
> +
> +	extclk_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 +1294,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 +1318,16 @@ static int mt9m111_probe(struct i2c_client *client)
>  				BIT(V4L2_COLORFX_NEGATIVE) |
>  				BIT(V4L2_COLORFX_SOLARIZATION)),
>  			V4L2_COLORFX_NONE);

Empty line maybe ?

> +	/*
> +	 * The extclk rate equals the link freq. if reg default values are used,
> +	 * which is the case. This must be adapted as soon as we don't use the
> +	 * default values anymore.
> +	 */
> +	ctrl = v4l2_ctrl_new_int_menu(&mt9m111->hdl, &mt9m111_ctrl_ops,
> +				      V4L2_CID_LINK_FREQ, 0, 0, &extclk_rate);
> +	if (ctrl)
> +		ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +

I'm sorry I have not replied to your previous email about using
LINK_FREQ for parallel busses.. I see it mentioned in ext-ctrls-image-process.rst
as you said:

``V4L2_CID_LINK_FREQ (integer menu)``
    The frequency of the data bus (e.g. parallel or CSI-2).

I still have a bit of troubles seeing it apply nicely on a parallel
bus. Isn't PIXEL_RATE more appropriate ? You said you need to know the
overall bus bandwidth in bytes , and pixel_rate * bpp / 8 is equally
valid and easy as link_freq / num_lanes, which requires the receiver
to fetch the remote subdev media bus configuration instead of relying
on the input format. Also LINK_FREQ is a menu control, something nasty
already for CSI-2 busses, which requires to pre-calculate the link
freqs based on the input mclk. It is also meant to be changed by
userspace, while PIXEL_RATE is RO by default.

Sakari, Laurent, what's your take here ?



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

Re: [PATCH v2 1/3] media: mt9m111: add V4L2_CID_LINK_FREQ support

[Laurent Pinchart]

On Tue, Sep 20, 2022 at 11:43:37AM +0200, Jacopo Mondi wrote:
> On Fri, Sep 16, 2022 at 03:57:11PM +0200, Marco Felsch wrote:
> > Add support to report the link frequency.
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > The v1 of this small series can be found here:
> > https://lore.kernel.org/all/20220818144712.997477-1-m.felsch@pengutronix.de/
> >
> > Thanks a lot to Jacopo for the review feedback on my v1.
> >
> > Changelog:
> >
> > v2:
> > - use V4L2_CID_LINK_FREQ instead of V4L2_CID_PIXEL_RATE
> > ---
> >  drivers/media/i2c/mt9m111.c | 21 ++++++++++++++++++++-
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > index afc86efa9e3e..52be1c310455 100644
> > --- a/drivers/media/i2c/mt9m111.c
> > +++ b/drivers/media/i2c/mt9m111.c
> > @@ -1249,6 +1249,8 @@ static int mt9m111_probe(struct i2c_client *client)
> >  {
> >  	struct mt9m111 *mt9m111;
> >  	struct i2c_adapter *adapter = client->adapter;
> > +	static s64 extclk_rate;
> 
> Why static ?

I missed that one indeed. I assume it's static because the pointer is
stored in the v4l2_ctrl structure by v4l2_ctrl_new_int_menu(), but
that's wrong. The data should be in the mt9m111 structure instead,
otherwise it won't work right when using multiple sensors.

> Also clk_get_rate() returns an unsigned long

v4l2_ctrl_new_int_menu() requires an s64 pointer.

> > +	struct v4l2_ctrl *ctrl;
> >  	int ret;
> >
> >  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
> > @@ -1271,6 +1273,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;
> > +
> > +	extclk_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 +1294,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 +1318,16 @@ static int mt9m111_probe(struct i2c_client *client)
> >  				BIT(V4L2_COLORFX_NEGATIVE) |
> >  				BIT(V4L2_COLORFX_SOLARIZATION)),
> >  			V4L2_COLORFX_NONE);
> 
> Empty line maybe ?
> 
> > +	/*
> > +	 * The extclk rate equals the link freq. if reg default values are used,
> > +	 * which is the case. This must be adapted as soon as we don't use the
> > +	 * default values anymore.
> > +	 */
> > +	ctrl = v4l2_ctrl_new_int_menu(&mt9m111->hdl, &mt9m111_ctrl_ops,
> > +				      V4L2_CID_LINK_FREQ, 0, 0, &extclk_rate);
> > +	if (ctrl)
> > +		ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> 
> I'm sorry I have not replied to your previous email about using
> LINK_FREQ for parallel busses.. I see it mentioned in ext-ctrls-image-process.rst
> as you said:
> 
> ``V4L2_CID_LINK_FREQ (integer menu)``
>     The frequency of the data bus (e.g. parallel or CSI-2).
> 
> I still have a bit of troubles seeing it apply nicely on a parallel
> bus. Isn't PIXEL_RATE more appropriate ?

They are different. When transmitting YUYV_2X8 for instance, the link
frequency is twice the pixel clock rate.

> You said you need to know the
> overall bus bandwidth in bytes , and pixel_rate * bpp / 8 is equally
> valid and easy as link_freq / num_lanes, which requires the receiver
> to fetch the remote subdev media bus configuration instead of relying
> on the input format. Also LINK_FREQ is a menu control, something nasty
> already for CSI-2 busses, which requires to pre-calculate the link
> freqs based on the input mclk. It is also meant to be changed by
> userspace, while PIXEL_RATE is RO by default.
> 
> Sakari, Laurent, what's your take here ?

Ideally both should be implemented by the driver.

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

-- 
Regards,

Laurent Pinchart

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

[Jacopo Mondi]

Hi Marco

On Fri, Sep 16, 2022 at 03:57:12PM +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.
>
> Remove the hardware register access from the callbacks and instead sync
> the state once right before the stream gets enabled to fix this for
> .set_fmt() and .set_selection(). For the debug register access we need
> to turn the device on/off before accessing the registers to fix this and
> finally for the ctrls access we need the device to be powered to fix
> this.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> Changelog:
>
> v2:
> - squash patch 2 and 3
> ---
>  drivers/media/i2c/mt9m111.c | 40 ++++++++++++++++++++-----------------
>  1 file changed, 22 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index 52be1c310455..8de93ab99cbc 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,11 @@ 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;
> -	}
> +	mt9m111->rect = rect;
> +	mt9m111->width = width;
> +	mt9m111->height = height;
>
> -	return ret;
> +	return 0;
>  }
>
>  static int mt9m111_get_selection(struct v4l2_subdev *sd,
> @@ -632,7 +629,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 +677,11 @@ 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;
> -	}
> +	mt9m111->width	= mf->width;
> +	mt9m111->height	= mf->height;
> +	mt9m111->fmt	= fmt;
>
> -	return ret;
> +	return 0;
>  }
>
>  static const struct mt9m111_mode_info *
> @@ -748,6 +739,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)
> @@ -758,10 +751,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;
>
> @@ -776,9 +773,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);
> +

So far so good

>  	return 0;
>  }
>  #endif
> @@ -891,6 +892,9 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
>  	struct mt9m111 *mt9m111 = container_of(ctrl->handler,
>  					       struct mt9m111, hdl);
>
> +	if (!mt9m111->is_streaming)
> +		return 0;
> +

If you refuse to set controls if the sensor is not streaming (ie
powered) which I think it's right, shouldn't you call
__v4l2_ctrl_handler_setup() at s_stream(1) time to have the controls
applied ?

>  	switch (ctrl->id) {
>  	case V4L2_CID_VFLIP:
>  		return mt9m111_set_flip(mt9m111, ctrl->val,
> --
> 2.30.2
>

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

[Jacopo Mondi]

Hi Marco

On Tue, Sep 20, 2022 at 12:27:04PM +0200, Jacopo Mondi wrote:
> Hi Marco
>
> On Fri, Sep 16, 2022 at 03:57:12PM +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.
> >
> > Remove the hardware register access from the callbacks and instead sync
> > the state once right before the stream gets enabled to fix this for
> > .set_fmt() and .set_selection(). For the debug register access we need
> > to turn the device on/off before accessing the registers to fix this and
> > finally for the ctrls access we need the device to be powered to fix
> > this.
> >
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > Changelog:
> >
> > v2:
> > - squash patch 2 and 3
> > ---
> >  drivers/media/i2c/mt9m111.c | 40 ++++++++++++++++++++-----------------
> >  1 file changed, 22 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > index 52be1c310455..8de93ab99cbc 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,11 @@ 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;
> > -	}
> > +	mt9m111->rect = rect;
> > +	mt9m111->width = width;
> > +	mt9m111->height = height;
> >
> > -	return ret;
> > +	return 0;
> >  }
> >
> >  static int mt9m111_get_selection(struct v4l2_subdev *sd,
> > @@ -632,7 +629,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 +677,11 @@ 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;
> > -	}
> > +	mt9m111->width	= mf->width;
> > +	mt9m111->height	= mf->height;
> > +	mt9m111->fmt	= fmt;
> >
> > -	return ret;
> > +	return 0;
> >  }
> >
> >  static const struct mt9m111_mode_info *
> > @@ -748,6 +739,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)
> > @@ -758,10 +751,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;
> >
> > @@ -776,9 +773,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);
> > +
>
> So far so good
>
> >  	return 0;
> >  }
> >  #endif
> > @@ -891,6 +892,9 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
> >  	struct mt9m111 *mt9m111 = container_of(ctrl->handler,
> >  					       struct mt9m111, hdl);
> >
> > +	if (!mt9m111->is_streaming)
> > +		return 0;
> > +
>
> If you refuse to set controls if the sensor is not streaming (ie
> powered) which I think it's right, shouldn't you call
> __v4l2_ctrl_handler_setup() at s_stream(1) time to have the controls
> applied ?
>

Oh scratch that, it's in the s_power(1) call path. It's would be nicer
to have runtime_pm, but you already know that :)

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

> >  	switch (ctrl->id) {
> >  	case V4L2_CID_VFLIP:
> >  		return mt9m111_set_flip(mt9m111, ctrl->val,
> > --
> > 2.30.2
> >

Re: [PATCH v2 1/3] media: mt9m111: add V4L2_CID_LINK_FREQ support

[Sakari Ailus]

Hi Jacopo,

On Tue, Sep 20, 2022 at 11:19:33AM +0200, Jacopo Mondi wrote:
> Hello
> 
> On Tue, Sep 20, 2022 at 10:56:17AM +0200, Marco Felsch wrote:
> > Hi Sakari,
> >
> > On 22-09-19, Sakari Ailus wrote:
> >
> > ...
> >
> > > > > > +	ret = clk_prepare_enable(mt9m111->clk);
> > > > > > +	if (ret < 0)
> > > > > > +		return ret;
> > > > > > +
> > > > > > +	extclk_rate = clk_get_rate(mt9m111->clk);
> > > > > > +	clk_disable_unprepare(mt9m111->clk);
> > > > >
> > > > > I don't think you'll need to enable a clock to just get its frequency.
> > > >
> > > > The official API states that you need to turn on the clk before
> > > > requesting it and it makes sense. Also there is a new helper
> > > > devm_clk_get_enabled() which addresses simple clk usage since most of
> > > > drivers don't enable it before requesting the rate.
> 
> Had the same question on v1 and Marco pointed me to the clk_get_rate()
> documentation
> https://elixir.bootlin.com/linux/v6.0-rc1/source/include/linux/clk.h#L682
> 
> which indeed specifies
> "This is only valid once the clock source has been enabled."
> 
> However none (or very few) of the linux-media i2c drivers actually do
> that.

I'm not aware of any. That's indeed what the documentation says. Also
clk_enable() documentation says that "If the clock can not be
enabled/disabled, this should return success". So I wonder how much can
you trust it. ;-)

> 
> I have added in cc the clk framework maintainer to see if he can help
> shed some light on this

Thanks.

But yes, to make this work in general case, one would need a way to ensure
the frequency is the one assigned in DT and that it won't change.

Getting the frequency (in an unreliable way?) isn't perfect but better than
nothing. So far I haven't heard of issues in practice though.

-- 
Regards,

Sakari Ailus

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

[Laurent Pinchart]

On Tue, Sep 20, 2022 at 12:34:12PM +0200, Jacopo Mondi wrote:
> On Tue, Sep 20, 2022 at 12:27:04PM +0200, Jacopo Mondi wrote:
> > On Fri, Sep 16, 2022 at 03:57:12PM +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.
> > >
> > > Remove the hardware register access from the callbacks and instead sync
> > > the state once right before the stream gets enabled to fix this for
> > > .set_fmt() and .set_selection(). For the debug register access we need
> > > to turn the device on/off before accessing the registers to fix this and
> > > finally for the ctrls access we need the device to be powered to fix
> > > this.
> > >
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > > Changelog:
> > >
> > > v2:
> > > - squash patch 2 and 3
> > > ---
> > >  drivers/media/i2c/mt9m111.c | 40 ++++++++++++++++++++-----------------
> > >  1 file changed, 22 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > > index 52be1c310455..8de93ab99cbc 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,11 @@ 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;
> > > -	}
> > > +	mt9m111->rect = rect;
> > > +	mt9m111->width = width;
> > > +	mt9m111->height = height;
> > >
> > > -	return ret;
> > > +	return 0;
> > >  }
> > >
> > >  static int mt9m111_get_selection(struct v4l2_subdev *sd,
> > > @@ -632,7 +629,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 +677,11 @@ 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;
> > > -	}
> > > +	mt9m111->width	= mf->width;
> > > +	mt9m111->height	= mf->height;
> > > +	mt9m111->fmt	= fmt;
> > >
> > > -	return ret;
> > > +	return 0;
> > >  }
> > >
> > >  static const struct mt9m111_mode_info *
> > > @@ -748,6 +739,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)
> > > @@ -758,10 +751,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;
> > >
> > > @@ -776,9 +773,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);
> > > +
> >
> > So far so good
> >
> > >  	return 0;
> > >  }
> > >  #endif
> > > @@ -891,6 +892,9 @@ static int mt9m111_s_ctrl(struct v4l2_ctrl *ctrl)
> > >  	struct mt9m111 *mt9m111 = container_of(ctrl->handler,
> > >  					       struct mt9m111, hdl);
> > >
> > > +	if (!mt9m111->is_streaming)
> > > +		return 0;
> > > +
> >
> > If you refuse to set controls if the sensor is not streaming (ie
> > powered) which I think it's right, shouldn't you call
> > __v4l2_ctrl_handler_setup() at s_stream(1) time to have the controls
> > applied ?
> 
> Oh scratch that, it's in the s_power(1) call path. It's would be nicer
> to have runtime_pm, but you already know that :)

Runtime PM shouldn't be hard to implement, I'd really prefer that.

> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> > >  	switch (ctrl->id) {
> > >  	case V4L2_CID_VFLIP:
> > >  		return mt9m111_set_flip(mt9m111, ctrl->val,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/3] media: mt9m111: don't turn on the output while powering it

[Jacopo Mondi]

Hi Marco

On Fri, Sep 16, 2022 at 03:57:13PM +0200, Marco Felsch wrote:
> Currently the .s_power() turn on/off the device and enables/disables the
> sensor output. This is wrong since it should only handle the power not
> not the sensor output behaviour. Enabling the sensor output should be
> part of the .s_stream() callback.
>
> Fix this by adding mt9m111_set_output() which gets called by the
> .s_stream() callback and remove the output register bits from
> mt9m111_resume/suspend.
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

I can't say the driver is nice to read (it has been around since a
long time and comes from the soc_camera times) but this makes it nicer
and fixes an issue

With Laurent's comment on the disabling order clarified:

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

> ---
> Changelog:
>
> v2:
> - new patch, replaces: "media: mt9m111: remove .s_power callback"
> ---
>  drivers/media/i2c/mt9m111.c | 38 ++++++++++++++++++++++++++-----------
>  1 file changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index 8de93ab99cbc..2cc0b0da7636 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -426,10 +426,25 @@ static int mt9m111_setup_geometry(struct mt9m111 *mt9m111, struct v4l2_rect *rec
>  	return ret;
>  }
>
> -static int mt9m111_enable(struct mt9m111 *mt9m111)
> +static int mt9m111_set_output(struct mt9m111 *mt9m111, int on)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
> -	return reg_write(RESET, MT9M111_RESET_CHIP_ENABLE);
> +	int ret;
> +
> +	if (on) {
> +		ret = reg_clear(RESET, MT9M111_RESET_OUTPUT_DISABLE);
> +		if (ret)
> +			return ret;
> +
> +		return reg_set(RESET, MT9M111_RESET_CHIP_ENABLE);
> +	}
> +
> +	/* disable */
> +	ret = reg_set(RESET, MT9M111_RESET_OUTPUT_DISABLE);
> +	if (ret)
> +		return ret;
> +
> +	return reg_clear(RESET, MT9M111_RESET_CHIP_ENABLE);
>  }
>
>  static int mt9m111_reset(struct mt9m111 *mt9m111)
> @@ -927,10 +942,7 @@ static int mt9m111_suspend(struct mt9m111 *mt9m111)
>  	ret = reg_set(RESET, MT9M111_RESET_RESET_MODE);
>  	if (!ret)
>  		ret = reg_set(RESET, MT9M111_RESET_RESET_SOC |
> -			      MT9M111_RESET_OUTPUT_DISABLE |
>  			      MT9M111_RESET_ANALOG_STANDBY);
> -	if (!ret)
> -		ret = reg_clear(RESET, MT9M111_RESET_CHIP_ENABLE);
>
>  	return ret;
>  }
> @@ -951,9 +963,9 @@ static void mt9m111_restore_state(struct mt9m111 *mt9m111)
>
>  static int mt9m111_resume(struct mt9m111 *mt9m111)
>  {
> -	int ret = mt9m111_enable(mt9m111);
> -	if (!ret)
> -		ret = mt9m111_reset(mt9m111);
> +	int ret;
> +
> +	ret = mt9m111_reset(mt9m111);
>  	if (!ret)
>  		mt9m111_restore_state(mt9m111);
>
> @@ -965,9 +977,7 @@ static int mt9m111_init(struct mt9m111 *mt9m111)
>  	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
>  	int ret;
>
> -	ret = mt9m111_enable(mt9m111);
> -	if (!ret)
> -		ret = mt9m111_reset(mt9m111);
> +	ret = mt9m111_reset(mt9m111);
>  	if (!ret)
>  		ret = mt9m111_set_context(mt9m111, mt9m111->ctx);
>  	if (ret)
> @@ -1116,8 +1126,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;
> +
> +	ret = mt9m111_set_output(mt9m111, enable);
> +	if (ret)
> +		return ret;
>
>  	mt9m111->is_streaming = !!enable;
> +
>  	return 0;
>  }
>
> --
> 2.30.2
>

Re: [PATCH v2 1/3] media: mt9m111: add V4L2_CID_LINK_FREQ support

[Marco Felsch]

On 22-09-20, Laurent Pinchart wrote:
> On Tue, Sep 20, 2022 at 11:43:37AM +0200, Jacopo Mondi wrote:
> > On Fri, Sep 16, 2022 at 03:57:11PM +0200, Marco Felsch wrote:
> > > Add support to report the link frequency.
> > >
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> > > The v1 of this small series can be found here:
> > > https://lore.kernel.org/all/20220818144712.997477-1-m.felsch@pengutronix.de/
> > >
> > > Thanks a lot to Jacopo for the review feedback on my v1.
> > >
> > > Changelog:
> > >
> > > v2:
> > > - use V4L2_CID_LINK_FREQ instead of V4L2_CID_PIXEL_RATE
> > > ---
> > >  drivers/media/i2c/mt9m111.c | 21 ++++++++++++++++++++-
> > >  1 file changed, 20 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > > index afc86efa9e3e..52be1c310455 100644
> > > --- a/drivers/media/i2c/mt9m111.c
> > > +++ b/drivers/media/i2c/mt9m111.c
> > > @@ -1249,6 +1249,8 @@ static int mt9m111_probe(struct i2c_client *client)
> > >  {
> > >  	struct mt9m111 *mt9m111;
> > >  	struct i2c_adapter *adapter = client->adapter;
> > > +	static s64 extclk_rate;
> > 
> > Why static ?
> 
> I missed that one indeed. I assume it's static because the pointer is
> stored in the v4l2_ctrl structure by v4l2_ctrl_new_int_menu(), but
> that's wrong. The data should be in the mt9m111 structure instead,
> otherwise it won't work right when using multiple sensors.

Yes, thats the reason. Didn't had in mind, the fact of using multiple
sensor of the same type. Sry. I will move it to the driver struct of
course.

> > Also clk_get_rate() returns an unsigned long
> 
> v4l2_ctrl_new_int_menu() requires an s64 pointer.

Yes.

Regards,
  Marco

> > > +	struct v4l2_ctrl *ctrl;
> > >  	int ret;
> > >
> > >  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
> > > @@ -1271,6 +1273,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;
> > > +
> > > +	extclk_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 +1294,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 +1318,16 @@ static int mt9m111_probe(struct i2c_client *client)
> > >  				BIT(V4L2_COLORFX_NEGATIVE) |
> > >  				BIT(V4L2_COLORFX_SOLARIZATION)),
> > >  			V4L2_COLORFX_NONE);
> > 
> > Empty line maybe ?
> > 
> > > +	/*
> > > +	 * The extclk rate equals the link freq. if reg default values are used,
> > > +	 * which is the case. This must be adapted as soon as we don't use the
> > > +	 * default values anymore.
> > > +	 */
> > > +	ctrl = v4l2_ctrl_new_int_menu(&mt9m111->hdl, &mt9m111_ctrl_ops,
> > > +				      V4L2_CID_LINK_FREQ, 0, 0, &extclk_rate);
> > > +	if (ctrl)
> > > +		ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > +
> > 
> > I'm sorry I have not replied to your previous email about using
> > LINK_FREQ for parallel busses.. I see it mentioned in ext-ctrls-image-process.rst
> > as you said:
> > 
> > ``V4L2_CID_LINK_FREQ (integer menu)``
> >     The frequency of the data bus (e.g. parallel or CSI-2).
> > 
> > I still have a bit of troubles seeing it apply nicely on a parallel
> > bus. Isn't PIXEL_RATE more appropriate ?
> 
> They are different. When transmitting YUYV_2X8 for instance, the link
> frequency is twice the pixel clock rate.
> 
> > You said you need to know the
> > overall bus bandwidth in bytes , and pixel_rate * bpp / 8 is equally
> > valid and easy as link_freq / num_lanes, which requires the receiver
> > to fetch the remote subdev media bus configuration instead of relying
> > on the input format. Also LINK_FREQ is a menu control, something nasty
> > already for CSI-2 busses, which requires to pre-calculate the link
> > freqs based on the input mclk. It is also meant to be changed by
> > userspace, while PIXEL_RATE is RO by default.
> > 
> > Sakari, Laurent, what's your take here ?
> 
> Ideally both should be implemented by the driver.
> 
> > >  	mt9m111->subdev.ctrl_handler = &mt9m111->hdl;
> > >  	if (mt9m111->hdl.error) {
> > >  		ret = mt9m111->hdl.error;
> 
> -- 
> Regards,
> 
> Laurent Pinchart
>