| Message ID | 20220818144712.997477-1-m.felsch@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] media: mt9m111: add V4L2_CID_PIXEL_RATE support | expand |
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 >
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 > > >
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 > > > > >
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 > > >
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
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 > > > > >
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
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 > > > > > > > >
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 > > > > > > > > > > >
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 > > > > > > > > > > > > > > >
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;
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(-)