| Message ID | 20220916135713.143890-1-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] media: mt9m111: add V4L2_CID_LINK_FREQ support | expand |
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;
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;
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 >
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?
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
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
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 >
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;
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.
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 >
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;
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(-)