| Message ID | 20220721083540.1525-1-laurent.pinchart@ideasonboard.com |
|---|---|
| Headers | show |
| Series | media: i2c: imx290: Miscellaneous improvements | expand |
Hello Dave, Am Donnerstag, 21. Juli 2022, 18:19:56 CEST schrieb Dave Stevenson: > Hi Laurent and Alexander > > On Thu, 21 Jul 2022 at 12:08, Alexander Stein > > <alexander.stein@ew.tq-group.com> wrote: > > Hello Laurent, > > > > Am Donnerstag, 21. Juli 2022, 12:40:36 CEST schrieb Laurent Pinchart: > > > Hi Alexander, > > > > > > On Thu, Jul 21, 2022 at 12:08:50PM +0200, Alexander Stein wrote: > > > > Am Donnerstag, 21. Juli 2022, 10:35:37 CEST schrieb Laurent Pinchart: > > > > > Registers 0x3012, 0x3013 and 0x3480 are not documented and are set > > > > > in > > > > > the per-mode register arrays with values indentical for all modes. > > > > > Move > > > > > them to the common array. > > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > --- > > > > > > > > > > drivers/media/i2c/imx290.c | 8 ++------ > > > > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > > > > > index 78772c6327a2..fc6e87fada1c 100644 > > > > > --- a/drivers/media/i2c/imx290.c > > > > > +++ b/drivers/media/i2c/imx290.c > > > > > @@ -192,6 +192,7 @@ static const struct imx290_regval > > > > > imx290_global_init_settings[] = { { IMX290_REG_8BIT(0x300f), 0x00 }, > > > > > > > > > > { IMX290_REG_8BIT(0x3010), 0x21 }, > > > > > { IMX290_REG_8BIT(0x3012), 0x64 }, > > > > > > > > > > + { IMX290_REG_8BIT(0x3013), 0x00 }, > > > > > > > > > > { IMX290_REG_8BIT(0x3016), 0x09 }, > > > > > { IMX290_REG_8BIT(0x3070), 0x02 }, > > > > > { IMX290_REG_8BIT(0x3071), 0x11 }, > > > > > > > > > > @@ -230,6 +231,7 @@ static const struct imx290_regval > > > > > imx290_global_init_settings[] = { { IMX290_REG_8BIT(0x33b0), 0x50 }, > > > > > > > > > > { IMX290_REG_8BIT(0x33b2), 0x1a }, > > > > > { IMX290_REG_8BIT(0x33b3), 0x04 }, > > > > > > > > > > + { IMX290_REG_8BIT(0x3480), 0x49 }, > > > > > > > > > > }; > > > > > > > > > > static const struct imx290_regval imx290_1080p_settings[] = { > > > > > > > > > > @@ -239,15 +241,12 @@ static const struct imx290_regval > > > > > imx290_1080p_settings[] = { { IMX290_OPB_SIZE_V, 10 }, > > > > > > > > > > { IMX290_X_OUT_SIZE, 1920 }, > > > > > { IMX290_Y_OUT_SIZE, 1080 }, > > > > > > > > > > - { IMX290_REG_8BIT(0x3012), 0x64 }, > > > > > - { IMX290_REG_8BIT(0x3013), 0x00 }, > > > > > > > > > > { IMX290_INCKSEL1, 0x18 }, > > > > > { IMX290_INCKSEL2, 0x03 }, > > > > > { IMX290_INCKSEL3, 0x20 }, > > > > > { IMX290_INCKSEL4, 0x01 }, > > > > > { IMX290_INCKSEL5, 0x1a }, > > > > > { IMX290_INCKSEL6, 0x1a }, > > > > > > > > > > - { IMX290_REG_8BIT(0x3480), 0x49 }, > > > > > > > > > > /* data rate settings */ > > > > > { IMX290_REPETITION, 0x10 }, > > > > > { IMX290_TCLKPOST, 87 }, > > > > > > > > > > @@ -267,15 +266,12 @@ static const struct imx290_regval > > > > > imx290_720p_settings[] = { { IMX290_OPB_SIZE_V, 4 }, > > > > > > > > > > { IMX290_X_OUT_SIZE, 1280 }, > > > > > { IMX290_Y_OUT_SIZE, 720 }, > > > > > > > > > > - { IMX290_REG_8BIT(0x3012), 0x64 }, > > > > > - { IMX290_REG_8BIT(0x3013), 0x00 }, > > > > > > > > > > { IMX290_INCKSEL1, 0x20 }, > > > > > { IMX290_INCKSEL2, 0x00 }, > > > > > { IMX290_INCKSEL3, 0x20 }, > > > > > { IMX290_INCKSEL4, 0x01 }, > > > > > { IMX290_INCKSEL5, 0x1a }, > > > > > { IMX290_INCKSEL6, 0x1a }, > > > > > > > > > > - { IMX290_REG_8BIT(0x3480), 0x49 }, > > > > > > > > > > /* data rate settings */ > > > > > { IMX290_REPETITION, 0x10 }, > > > > > { IMX290_TCLKPOST, 79 }, > > > > > > > > 0x3480 is INCKSEL7 for imx327, not sure if that should be set yet for > > > > imx290 (only) driver, without proper imx327 support. > > > > > > Do you mean the register doesn't exist on the IMX290 ? We could make it > > > conditional on the sensor model, but it's not added by this patch, it > > > has been there since the first version of the driver, so I'd rather do > > > that on top. > > > > As far as I know INCKSEL7 is only valid on imx327. On imx290 the whole > > 0x300-0x34ff range is reserved. > > IMX290_CSI_LANE_MODE to select the number of CSI2 data lanes is > 0x3443, so clearly the whole range is not reserved. You are right, I was looking at the wrong table, my bad. > > I agree this should be conditional on the sensor model. If you want to > > keep > > it, because it is not new, I'm fine with that. > > 0x3840 is documented in my IMX290 datasheet as being INCKSEL7. 0x49 > for 37.125MHz clock. 0x92 for 74.25MHz (default). > Removing it *will* break this driver for existing platforms as the > rest of the code configures a 37.125MHz clock. I guess you mean 0x3480, just a small typo. Just to be sure. Interesting, the datasheet for imx290 I found doesn't contain INCKSEL7 at all. But good to know, that this register applies to imx290 as well. Thanks for that information. Best regards, Alexander > See [1] for adding 74.25MHz clock support. > > Dave > > [1] > https://github.com/raspberrypi/linux/commit/125f7e7ef1194d4849c74b25c87d18a > ea9de2de7
Hi Sakari, On Fri, Jul 22, 2022 at 05:37:53PM +0300, Sakari Ailus wrote: > Hi Alexander, > > On Thu, Jul 21, 2022 at 01:43:54PM +0200, Alexander Stein wrote: > ... > > Nice the following snippet does the trick already: > > ---8<--- > > --- a/drivers/media/i2c/imx290.c > > +++ b/drivers/media/i2c/imx290.c > > @@ -221,6 +221,7 @@ static const struct imx290_pixfmt imx290_formats[] = { > > static const struct regmap_config imx290_regmap_config = { > > .reg_bits = 16, > > .val_bits = 8, > > + .use_single_read = true, > > }; > > > > static const char * const imx290_test_pattern_menu[] = { > > ---8<--- > > > > As this affects the VC OV9281 as well, any suggestions for a common property? > > If there's a 1:1 I²C mux in there between the host and the sensor, should > it be in DT as well? I'm not entirely certain it's necessary. The microcontroller also the sensor clock and power supplies, so it has to be modelled in DT in any case. I was trying to avoid exposing it as an I2C mux, but maybe we'll have to bite the bullet... I've implement support for two camera modules from Vision Components but haven't submitted patches yet. See [1] and [2] for DT examples and [3] for the driver that handles the microcontroller. Note that one purpose of the microcontroller is to configure the sensor automatically. It can be queried through I2C for a list of supported modes, and it can also reconfigure the sensor fully when a mode is selected. This is meant to enable development of a single driver that will cover all modules, regardless of which camera sensor it integrates. I'm not sure what words you will use to voice your opinion on this design, but I think I already agree :-) [1] https://gitlab.com/ideasonboard/nxp/linux/-/blob/pinchartl/v5.19/dev/isp/next/arch/arm64/boot/dts/freescale/imx8mp-maivin-csi1-imx296.dts [2] https://gitlab.com/ideasonboard/nxp/linux/-/blob/pinchartl/v5.19/dev/isp/next/arch/arm64/boot/dts/freescale/imx8mp-maivin-csi1-imx327.dts [3] https://gitlab.com/ideasonboard/nxp/linux/-/blob/pinchartl/v5.19/dev/isp/next/drivers/media/i2c/vc-mipi.c > The property could be called e.g. "single-octet-read". I think this should > probably be documented in I²C bindings (or even regmap). I like the idea of making it a DT property global to all I2C devices. It should ideally be parsed by the I2C core or by regmap.
Hi Laurent & Sakari, Am Sonntag, 24. Juli 2022, 01:06:29 CEST schrieb Laurent Pinchart: > Hi Sakari, > > On Fri, Jul 22, 2022 at 05:37:53PM +0300, Sakari Ailus wrote: > > Hi Alexander, > > > > On Thu, Jul 21, 2022 at 01:43:54PM +0200, Alexander Stein wrote: > > ... > > > > > Nice the following snippet does the trick already: > > > ---8<--- > > > --- a/drivers/media/i2c/imx290.c > > > +++ b/drivers/media/i2c/imx290.c > > > @@ -221,6 +221,7 @@ static const struct imx290_pixfmt imx290_formats[] = > > > { > > > > > > static const struct regmap_config imx290_regmap_config = { > > > > > > .reg_bits = 16, > > > .val_bits = 8, > > > > > > + .use_single_read = true, > > > > > > }; > > > > > > static const char * const imx290_test_pattern_menu[] = { > > > > > > ---8<--- > > > > > > As this affects the VC OV9281 as well, any suggestions for a common > > > property?> > > If there's a 1:1 I²C mux in there between the host and the sensor, should > > it be in DT as well? I'm not entirely certain it's necessary. > > The microcontroller also the sensor clock and power supplies, so it has > to be modelled in DT in any case. I was trying to avoid exposing it as > an I2C mux, but maybe we'll have to bite the bullet... What is the benefit about exposing a I2C mux? The needed regmap config option is configured completely independent to this. > I've implement support for two camera modules from Vision Components but > haven't submitted patches yet. See [1] and [2] for DT examples and [3] > for the driver that handles the microcontroller. > > Note that one purpose of the microcontroller is to configure the sensor > automatically. It can be queried through I2C for a list of supported > modes, and it can also reconfigure the sensor fully when a mode is > selected. This is meant to enable development of a single driver that > will cover all modules, regardless of which camera sensor it integrates. > I'm not sure what words you will use to voice your opinion on this > design, but I think I already agree :-) > > [1] > https://gitlab.com/ideasonboard/nxp/linux/-/blob/pinchartl/v5.19/dev/isp/ne > xt/arch/arm64/boot/dts/freescale/imx8mp-maivin-csi1-imx296.dts [2] > https://gitlab.com/ideasonboard/nxp/linux/-/blob/pinchartl/v5.19/dev/isp/ne > xt/arch/arm64/boot/dts/freescale/imx8mp-maivin-csi1-imx327.dts [3] > https://gitlab.com/ideasonboard/nxp/linux/-/blob/pinchartl/v5.19/dev/isp/ne > xt/drivers/media/i2c/vc-mipi.c > > The property could be called e.g. "single-octet-read". I think this should > > probably be documented in I²C bindings (or even regmap). > > I like the idea of making it a DT property global to all I2C devices. It > should ideally be parsed by the I2C core or by regmap. I agree with adding this as a regmap option, like 'big-endian' & friends, but not so much for I2C core. IMHO the core should only be interested in handling messages and transfers. Setting up those correctly is a matter for drivers (which in turn use regmap). Best regards, Alexander
Hi Alexander, On Mon, Jul 25, 2022 at 08:49:40AM +0200, Alexander Stein wrote: > Am Sonntag, 24. Juli 2022, 01:06:29 CEST schrieb Laurent Pinchart: > > On Fri, Jul 22, 2022 at 05:37:53PM +0300, Sakari Ailus wrote: > > > On Thu, Jul 21, 2022 at 01:43:54PM +0200, Alexander Stein wrote: > > > ... > > > > > > > Nice the following snippet does the trick already: > > > > ---8<--- > > > > --- a/drivers/media/i2c/imx290.c > > > > +++ b/drivers/media/i2c/imx290.c > > > > @@ -221,6 +221,7 @@ static const struct imx290_pixfmt imx290_formats[] = > > > > { > > > > static const struct regmap_config imx290_regmap_config = { > > > > .reg_bits = 16, > > > > .val_bits = 8, > > > > + .use_single_read = true, > > > > }; > > > > > > > > static const char * const imx290_test_pattern_menu[] = { > > > > > > > > ---8<--- > > > > > > > > As this affects the VC OV9281 as well, any suggestions for a common > > > > property? > > > > > > If there's a 1:1 I²C mux in there between the host and the sensor, should > > > it be in DT as well? I'm not entirely certain it's necessary. > > > > The microcontroller also the sensor clock and power supplies, so it has > > to be modelled in DT in any case. I was trying to avoid exposing it as > > an I2C mux, but maybe we'll have to bite the bullet... > > What is the benefit about exposing a I2C mux? The needed regmap config option > is configured completely independent to this. If the I2C mux in the camera module messes up I2C transfers, the related quirks need to be handled somewhere, and a specific mux driver device in DT could be a good place to report that. There may be other options though. > > I've implement support for two camera modules from Vision Components but > > haven't submitted patches yet. See [1] and [2] for DT examples and [3] > > for the driver that handles the microcontroller. > > > > Note that one purpose of the microcontroller is to configure the sensor > > automatically. It can be queried through I2C for a list of supported > > modes, and it can also reconfigure the sensor fully when a mode is > > selected. This is meant to enable development of a single driver that > > will cover all modules, regardless of which camera sensor it integrates. > > I'm not sure what words you will use to voice your opinion on this > > design, but I think I already agree :-) > > > > [1] https://gitlab.com/ideasonboard/nxp/linux/-/blob/pinchartl/v5.19/dev/isp/next/arch/arm64/boot/dts/freescale/imx8mp-maivin-csi1-imx296.dts > > [2] https://gitlab.com/ideasonboard/nxp/linux/-/blob/pinchartl/v5.19/dev/isp/next/arch/arm64/boot/dts/freescale/imx8mp-maivin-csi1-imx327.dts > > [3] https://gitlab.com/ideasonboard/nxp/linux/-/blob/pinchartl/v5.19/dev/isp/next/drivers/media/i2c/vc-mipi.c > > > > > The property could be called e.g. "single-octet-read". I think this should > > > probably be documented in I²C bindings (or even regmap). > > > > I like the idea of making it a DT property global to all I2C devices. It > > should ideally be parsed by the I2C core or by regmap. > > I agree with adding this as a regmap option, like 'big-endian' & friends, but > not so much for I2C core. IMHO the core should only be interested in handling > messages and transfers. Setting up those correctly is a matter for drivers > (which in turn use regmap). I don't want to polute a large number of sensor drivers because of questionable design decisions of a particular module vendor. This type of quirk needs to be handled outside of the sensor driver.
Hi Alexander, On Tue, Aug 23, 2022 at 04:08:20AM +0300, Laurent Pinchart wrote: > On Mon, Jul 25, 2022 at 08:49:40AM +0200, Alexander Stein wrote: > > Am Sonntag, 24. Juli 2022, 01:06:29 CEST schrieb Laurent Pinchart: > > > On Fri, Jul 22, 2022 at 05:37:53PM +0300, Sakari Ailus wrote: > > > > On Thu, Jul 21, 2022 at 01:43:54PM +0200, Alexander Stein wrote: > > > > ... > > > > > > > > > Nice the following snippet does the trick already: > > > > > ---8<--- > > > > > --- a/drivers/media/i2c/imx290.c > > > > > +++ b/drivers/media/i2c/imx290.c > > > > > @@ -221,6 +221,7 @@ static const struct imx290_pixfmt imx290_formats[] = > > > > > { > > > > > static const struct regmap_config imx290_regmap_config = { > > > > > .reg_bits = 16, > > > > > .val_bits = 8, > > > > > + .use_single_read = true, > > > > > }; > > > > > > > > > > static const char * const imx290_test_pattern_menu[] = { > > > > > > > > > > ---8<--- > > > > > > > > > > As this affects the VC OV9281 as well, any suggestions for a common > > > > > property? > > > > > > > > If there's a 1:1 I²C mux in there between the host and the sensor, should > > > > it be in DT as well? I'm not entirely certain it's necessary. > > > > > > The microcontroller also the sensor clock and power supplies, so it has > > > to be modelled in DT in any case. I was trying to avoid exposing it as > > > an I2C mux, but maybe we'll have to bite the bullet... > > > > What is the benefit about exposing a I2C mux? The needed regmap config option > > is configured completely independent to this. > > If the I2C mux in the camera module messes up I2C transfers, the related > quirks need to be handled somewhere, and a specific mux driver device in > DT could be a good place to report that. There may be other options > though. > > > > I've implement support for two camera modules from Vision Components but > > > haven't submitted patches yet. See [1] and [2] for DT examples and [3] > > > for the driver that handles the microcontroller. > > > > > > Note that one purpose of the microcontroller is to configure the sensor > > > automatically. It can be queried through I2C for a list of supported > > > modes, and it can also reconfigure the sensor fully when a mode is > > > selected. This is meant to enable development of a single driver that > > > will cover all modules, regardless of which camera sensor it integrates. > > > I'm not sure what words you will use to voice your opinion on this > > > design, but I think I already agree :-) > > > > > > [1] https://gitlab.com/ideasonboard/nxp/linux/-/blob/pinchartl/v5.19/dev/isp/next/arch/arm64/boot/dts/freescale/imx8mp-maivin-csi1-imx296.dts > > > [2] https://gitlab.com/ideasonboard/nxp/linux/-/blob/pinchartl/v5.19/dev/isp/next/arch/arm64/boot/dts/freescale/imx8mp-maivin-csi1-imx327.dts > > > [3] https://gitlab.com/ideasonboard/nxp/linux/-/blob/pinchartl/v5.19/dev/isp/next/drivers/media/i2c/vc-mipi.c > > > > > > > The property could be called e.g. "single-octet-read". I think this should > > > > probably be documented in I²C bindings (or even regmap). > > > > > > I like the idea of making it a DT property global to all I2C devices. It > > > should ideally be parsed by the I2C core or by regmap. > > > > I agree with adding this as a regmap option, like 'big-endian' & friends, but > > not so much for I2C core. IMHO the core should only be interested in handling > > messages and transfers. Setting up those correctly is a matter for drivers > > (which in turn use regmap). > > I don't want to polute a large number of sensor drivers because of > questionable design decisions of a particular module vendor. This type > of quirk needs to be handled outside of the sensor driver. Given that the chip ID is only read to print it to the kernel log, and that an incorrectly read ID will not prevent the driver from probing or affect its behaviour in any way, would you object to merging this patch, with the single read issue to support the Vision Components module being handled later ?
Hello Laurent, Am Dienstag, 23. August 2022, 04:51:20 CEST schrieb Laurent Pinchart: > Hi Alexander, > > On Tue, Aug 23, 2022 at 04:08:20AM +0300, Laurent Pinchart wrote: > > On Mon, Jul 25, 2022 at 08:49:40AM +0200, Alexander Stein wrote: > > > Am Sonntag, 24. Juli 2022, 01:06:29 CEST schrieb Laurent Pinchart: > > > > On Fri, Jul 22, 2022 at 05:37:53PM +0300, Sakari Ailus wrote: > > > > > On Thu, Jul 21, 2022 at 01:43:54PM +0200, Alexander Stein wrote: > > > > > ... > > > > > > > > > > > Nice the following snippet does the trick already: > > > > > > ---8<--- > > > > > > --- a/drivers/media/i2c/imx290.c > > > > > > +++ b/drivers/media/i2c/imx290.c > > > > > > @@ -221,6 +221,7 @@ static const struct imx290_pixfmt > > > > > > imx290_formats[] = > > > > > > { > > > > > > > > > > > > static const struct regmap_config imx290_regmap_config = { > > > > > > > > > > > > .reg_bits = 16, > > > > > > .val_bits = 8, > > > > > > > > > > > > + .use_single_read = true, > > > > > > > > > > > > }; > > > > > > > > > > > > static const char * const imx290_test_pattern_menu[] = { > > > > > > > > > > > > ---8<--- > > > > > > > > > > > > As this affects the VC OV9281 as well, any suggestions for a > > > > > > common > > > > > > property? > > > > > > > > > > If there's a 1:1 I²C mux in there between the host and the sensor, > > > > > should > > > > > it be in DT as well? I'm not entirely certain it's necessary. > > > > > > > > The microcontroller also the sensor clock and power supplies, so it > > > > has > > > > to be modelled in DT in any case. I was trying to avoid exposing it as > > > > an I2C mux, but maybe we'll have to bite the bullet... > > > > > > What is the benefit about exposing a I2C mux? The needed regmap config > > > option is configured completely independent to this. > > > > If the I2C mux in the camera module messes up I2C transfers, the related > > quirks need to be handled somewhere, and a specific mux driver device in > > DT could be a good place to report that. There may be other options > > though. From a logical point of view, a i2c mux seems to be correct, but in the end this quirk is handled by regmap which parses device specific properties. Adding a (mux) bus property which is applied to all devices seems to be a hassle, IMHO. Taking Sakari's suggestion of 'single-octet-read' property where in the DT bindings this should be added? > > > > I've implement support for two camera modules from Vision Components > > > > but > > > > haven't submitted patches yet. See [1] and [2] for DT examples and [3] > > > > for the driver that handles the microcontroller. > > > > > > > > Note that one purpose of the microcontroller is to configure the > > > > sensor > > > > automatically. It can be queried through I2C for a list of supported > > > > modes, and it can also reconfigure the sensor fully when a mode is > > > > selected. This is meant to enable development of a single driver that > > > > will cover all modules, regardless of which camera sensor it > > > > integrates. > > > > I'm not sure what words you will use to voice your opinion on this > > > > design, but I think I already agree :-) > > > > > > > > [1] > > > > https://gitlab.com/ideasonboard/nxp/linux/-/blob/pinchartl/v5.19/dev/ > > > > isp/next/arch/arm64/boot/dts/freescale/imx8mp-maivin-csi1-imx296.dts > > > > [2] > > > > https://gitlab.com/ideasonboard/nxp/linux/-/blob/pinchartl/v5.19/dev/ > > > > isp/next/arch/arm64/boot/dts/freescale/imx8mp-maivin-csi1-imx327.dts > > > > [3] > > > > https://gitlab.com/ideasonboard/nxp/linux/-/blob/pinchartl/v5.19/dev/ > > > > isp/next/drivers/media/i2c/vc-mipi.c> > > > > > > > The property could be called e.g. "single-octet-read". I think this > > > > > should > > > > > probably be documented in I²C bindings (or even regmap). > > > > > > > > I like the idea of making it a DT property global to all I2C devices. > > > > It > > > > should ideally be parsed by the I2C core or by regmap. > > > > > > I agree with adding this as a regmap option, like 'big-endian' & > > > friends, but not so much for I2C core. IMHO the core should only be > > > interested in handling messages and transfers. Setting up those > > > correctly is a matter for drivers (which in turn use regmap). > > > > I don't want to polute a large number of sensor drivers because of > > questionable design decisions of a particular module vendor. This type > > of quirk needs to be handled outside of the sensor driver. > > Given that the chip ID is only read to print it to the kernel log, and > that an incorrectly read ID will not prevent the driver from probing or > affect its behaviour in any way, would you object to merging this patch, > with the single read issue to support the Vision Components module being > handled later ? No objection here. This problem is and should stay outside of the sensor driver. VC platform integration is an additional step. Best regards, Alexander
On Thu, Jul 21, 2022 at 01:28:34PM +0200, Alexander Stein wrote: > Am Donnerstag, 21. Juli 2022, 13:08:05 CEST schrieb Laurent Pinchart: > > On Thu, Jul 21, 2022 at 12:00:55PM +0200, Alexander Stein wrote: > > > Am Donnerstag, 21. Juli 2022, 10:35:31 CEST schrieb Laurent Pinchart: > > > > Define macros for all registers programmed by the driver for which > > > > documentation is available to increase readability. This starts making > > > > use of 16-bit registers in the register arrays, so the value field has > > > > to be increased to 32 bits. > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > > > > > drivers/media/i2c/imx290.c | 219 +++++++++++++++++++++---------------- > > > > 1 file changed, 124 insertions(+), 95 deletions(-) > > > > > > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > > > > index 5b7f9027b50f..bec326a83952 100644 > > > > --- a/drivers/media/i2c/imx290.c > > > > +++ b/drivers/media/i2c/imx290.c > > > > @@ -31,14 +31,73 @@ > > > > #define IMX290_STANDBY IMX290_REG_8BIT(0x3000) > > > > #define IMX290_REGHOLD IMX290_REG_8BIT(0x3001) > > > > #define IMX290_XMSTA IMX290_REG_8BIT(0x3002) > > > > +#define IMX290_ADBIT IMX290_REG_8BIT(0x3005) > > > > +#define IMX290_ADBIT_10BIT (0 << 0) > > > > +#define IMX290_ADBIT_12BIT (1 << 0) > > > > +#define IMX290_CTRL_07 IMX290_REG_8BIT(0x3007) > > > > +#define IMX290_VREVERSE BIT(0) > > > > +#define IMX290_HREVERSE BIT(1) > > > > +#define IMX290_WINMODE_1080P (0 << 4) > > > > +#define IMX290_WINMODE_720P (1 << 4) > > > > +#define IMX290_WINMODE_CROP (4 << 4) > > > > #define IMX290_FR_FDG_SEL IMX290_REG_8BIT(0x3009) > > > > #define IMX290_BLKLEVEL IMX290_REG_16BIT(0x300a) > > > > #define IMX290_GAIN IMX290_REG_8BIT(0x3014) > > > > +#define IMX290_VMAX IMX290_REG_24BIT(0x3018) > > > > #define IMX290_HMAX IMX290_REG_16BIT(0x301c) > > > > +#define IMX290_SHS1 IMX290_REG_24BIT(0x3020) > > > > +#define IMX290_WINWV_OB IMX290_REG_8BIT(0x303a) > > > > +#define IMX290_WINPV IMX290_REG_16BIT(0x303c) > > > > +#define IMX290_WINWV IMX290_REG_16BIT(0x303e) > > > > +#define IMX290_WINPH IMX290_REG_16BIT(0x3040) > > > > +#define IMX290_WINWH IMX290_REG_16BIT(0x3042) > > > > +#define IMX290_OUT_CTRL IMX290_REG_8BIT(0x3046) > > > > +#define IMX290_ODBIT_10BIT (0 << 0) > > > > +#define IMX290_ODBIT_12BIT (1 << 0) > > > > > > ODBIT is fixed 1h for MIPI-CSI-2 output. > > > > > > > +#define IMX290_OPORTSEL_PARALLEL (0x0 << 4) > > > > +#define IMX290_OPORTSEL_LVDS_2CH (0xd << 4) > > > > +#define IMX290_OPORTSEL_LVDS_4CH (0xe << 4) > > > > +#define IMX290_OPORTSEL_LVDS_8CH (0xf << 4) > > > > > > This driver only supports MIPI-CSI-2 output, but these bits are don't care, > > > you still want to list them here for completeness? > > > > Yes, it could be useful later. > > Ok, fine with me. > > > > > +#define IMX290_XSOUTSEL IMX290_REG_8BIT(0x304b) > > > > +#define IMX290_XSOUTSEL_XVSOUTSEL_HIGH (0 << 0) > > > > +#define IMX290_XSOUTSEL_XVSOUTSEL_VSYNC (2 << 0) > > > > +#define IMX290_XSOUTSEL_XHSOUTSEL_HIGH (0 << 2) > > > > +#define IMX290_XSOUTSEL_XHSOUTSEL_HSYNC (2 << 2) > > > > +#define IMX290_INCKSEL1 IMX290_REG_8BIT(0x305c) > > > > +#define IMX290_INCKSEL2 IMX290_REG_8BIT(0x305d) > > > > +#define IMX290_INCKSEL3 IMX290_REG_8BIT(0x305e) > > > > +#define IMX290_INCKSEL4 IMX290_REG_8BIT(0x305f) > > > > #define IMX290_PGCTRL IMX290_REG_8BIT(0x308c) > > > > +#define IMX290_ADBIT1 IMX290_REG_8BIT(0x3129) > > > > +#define IMX290_ADBIT1_10BIT 0x1d > > > > +#define IMX290_ADBIT1_12BIT 0x00 > > > > +#define IMX290_INCKSEL5 IMX290_REG_8BIT(0x315e) > > > > +#define IMX290_INCKSEL6 IMX290_REG_8BIT(0x3164) > > > > > > Any reason to skip the bit defines for both supported input clocks? > > > > I don't have access to information that describes the bits in those two > > registers, just magic values for the 37.125 MHz and 74.25 MHz input > > clocks. Would you happen to know more ? > > See [1] and line 354 as well. This register is depending on the external > clock. > > [1] https://github.com/tq-systems/linux-tqmaxx/blob/TQMa8-fslc-5.10-2.1.x-imx/drivers/media/i2c/imx290.c#L344 Right, but I don't know what those values mean. I'm defining in this patch macros for register addresses and bits that have a known name. Magic values are left as hex constants in the register arrays or code. A subsequent patch could address that if desired. > > > > +#define IMX290_ADBIT2 IMX290_REG_8BIT(0x317c) > > > > +#define IMX290_ADBIT2_10BIT 0x12 > > > > +#define IMX290_ADBIT2_12BIT 0x00 > > > > #define IMX290_CHIP_ID IMX290_REG_16BIT(0x319a) > > > > +#define IMX290_ADBIT3 IMX290_REG_16BIT(0x31ec) > > > > +#define IMX290_ADBIT3_10BIT 0x37 > > > > +#define IMX290_ADBIT3_12BIT 0x0e > > > > +#define IMX290_REPETITION IMX290_REG_8BIT(0x3405) > > > > #define IMX290_PHY_LANE_NUM IMX290_REG_8BIT(0x3407) > > > > +#define IMX290_OPB_SIZE_V IMX290_REG_8BIT(0x3414) > > > > +#define IMX290_Y_OUT_SIZE IMX290_REG_16BIT(0x3418) > > > > +#define IMX290_CSI_DT_FMT IMX290_REG_16BIT(0x3441) > > > > +#define IMX290_CSI_DT_FMT_RAW10 0x0a0a > > > > +#define IMX290_CSI_DT_FMT_RAW12 0x0c0c > > > > #define IMX290_CSI_LANE_MODE IMX290_REG_8BIT(0x3443) > > > > +#define IMX290_EXTCK_FREQ IMX290_REG_16BIT(0x3444) > > > > > > Same here. > > > > Same explanation as above :-) > > This register also depends on external clock, see [2] and line 228 as well. > > [2] https://github.com/tq-systems/linux-tqmaxx/blob/TQMa8-fslc-5.10-2.1.x-imx/drivers/media/i2c/imx290.c#L218 > > > > > +#define IMX290_TCLKPOST IMX290_REG_16BIT(0x3446) > > > > +#define IMX290_THSZERO IMX290_REG_16BIT(0x3448) > > > > +#define IMX290_THSPREPARE IMX290_REG_16BIT(0x344a) > > > > +#define IMX290_TCLKTRAIL IMX290_REG_16BIT(0x344c) > > > > +#define IMX290_THSTRAIL IMX290_REG_16BIT(0x344e) > > > > +#define IMX290_TCLKZERO IMX290_REG_16BIT(0x3450) > > > > +#define IMX290_TCLKPREPARE IMX290_REG_16BIT(0x3452) > > > > +#define IMX290_TLPX IMX290_REG_16BIT(0x3454) > > > > +#define IMX290_X_OUT_SIZE IMX290_REG_16BIT(0x3472) > > > > > > > > #define IMX290_PGCTRL_REGEN BIT(0) > > > > #define IMX290_PGCTRL_THRU BIT(1) > > > > > > > > @@ -54,7 +113,7 @@ static const char * const imx290_supply_name[] = { > > > > > > > > struct imx290_regval { > > > > u32 reg; > > > > - u8 val; > > > > + u32 val; > > > > }; > > > > > > > > struct imx290_mode { > > > > @@ -116,22 +175,16 @@ static const char * const imx290_test_pattern_menu[] = { }; > > > > > > > > static const struct imx290_regval imx290_global_init_settings[] = { > > > > > > > > - { IMX290_REG_8BIT(0x3007), 0x00 }, > > > > - { IMX290_REG_8BIT(0x3018), 0x65 }, > > > > - { IMX290_REG_8BIT(0x3019), 0x04 }, > > > > - { IMX290_REG_8BIT(0x301a), 0x00 }, > > > > - { IMX290_REG_8BIT(0x3444), 0x20 }, > > > > - { IMX290_REG_8BIT(0x3445), 0x25 }, > > > > - { IMX290_REG_8BIT(0x303a), 0x0c }, > > > > - { IMX290_REG_8BIT(0x3040), 0x00 }, > > > > - { IMX290_REG_8BIT(0x3041), 0x00 }, > > > > - { IMX290_REG_8BIT(0x303c), 0x00 }, > > > > - { IMX290_REG_8BIT(0x303d), 0x00 }, > > > > - { IMX290_REG_8BIT(0x3042), 0x9c }, > > > > - { IMX290_REG_8BIT(0x3043), 0x07 }, > > > > - { IMX290_REG_8BIT(0x303e), 0x49 }, > > > > - { IMX290_REG_8BIT(0x303f), 0x04 }, > > > > - { IMX290_REG_8BIT(0x304b), 0x0a }, > > > > + { IMX290_CTRL_07, IMX290_WINMODE_1080P }, > > > > + { IMX290_VMAX, 1125 }, > > > > + { IMX290_EXTCK_FREQ, 0x2520 }, > > > > + { IMX290_WINWV_OB, 12 }, > > > > + { IMX290_WINPH, 0 }, > > > > + { IMX290_WINPV, 0 }, > > > > + { IMX290_WINWH, 1948 }, > > > > + { IMX290_WINWV, 1097 }, > > > > + { IMX290_XSOUTSEL, IMX290_XSOUTSEL_XVSOUTSEL_VSYNC | > > > > + IMX290_XSOUTSEL_XHSOUTSEL_HSYNC }, > > > > { IMX290_REG_8BIT(0x300f), 0x00 }, > > > > { IMX290_REG_8BIT(0x3010), 0x21 }, > > > > { IMX290_REG_8BIT(0x3012), 0x64 }, > > > > @@ -177,102 +230,78 @@ static const struct imx290_regval imx290_global_init_settings[] = { > > > > > > > > static const struct imx290_regval imx290_1080p_settings[] = { > > > > /* mode settings */ > > > > - { IMX290_REG_8BIT(0x3007), 0x00 }, > > > > - { IMX290_REG_8BIT(0x303a), 0x0c }, > > > > - { IMX290_REG_8BIT(0x3414), 0x0a }, > > > > - { IMX290_REG_8BIT(0x3472), 0x80 }, > > > > - { IMX290_REG_8BIT(0x3473), 0x07 }, > > > > - { IMX290_REG_8BIT(0x3418), 0x38 }, > > > > - { IMX290_REG_8BIT(0x3419), 0x04 }, > > > > + { IMX290_CTRL_07, IMX290_WINMODE_1080P }, > > > > + { IMX290_WINWV_OB, 12 }, > > > > + { IMX290_OPB_SIZE_V, 10 }, > > > > + { IMX290_X_OUT_SIZE, 1920 }, > > > > + { IMX290_Y_OUT_SIZE, 1080 }, > > > > { IMX290_REG_8BIT(0x3012), 0x64 }, > > > > { IMX290_REG_8BIT(0x3013), 0x00 }, > > > > - { IMX290_REG_8BIT(0x305c), 0x18 }, > > > > - { IMX290_REG_8BIT(0x305d), 0x03 }, > > > > - { IMX290_REG_8BIT(0x305e), 0x20 }, > > > > - { IMX290_REG_8BIT(0x305f), 0x01 }, > > > > - { IMX290_REG_8BIT(0x315e), 0x1a }, > > > > - { IMX290_REG_8BIT(0x3164), 0x1a }, > > > > + { IMX290_INCKSEL1, 0x18 }, > > > > + { IMX290_INCKSEL2, 0x03 }, > > > > + { IMX290_INCKSEL3, 0x20 }, > > > > + { IMX290_INCKSEL4, 0x01 }, > > > > + { IMX290_INCKSEL5, 0x1a }, > > > > + { IMX290_INCKSEL6, 0x1a }, > > > > { IMX290_REG_8BIT(0x3480), 0x49 }, > > > > /* data rate settings */ > > > > - { IMX290_REG_8BIT(0x3405), 0x10 }, > > > > - { IMX290_REG_8BIT(0x3446), 0x57 }, > > > > - { IMX290_REG_8BIT(0x3447), 0x00 }, > > > > - { IMX290_REG_8BIT(0x3448), 0x37 }, > > > > - { IMX290_REG_8BIT(0x3449), 0x00 }, > > > > - { IMX290_REG_8BIT(0x344a), 0x1f }, > > > > - { IMX290_REG_8BIT(0x344b), 0x00 }, > > > > - { IMX290_REG_8BIT(0x344c), 0x1f }, > > > > - { IMX290_REG_8BIT(0x344d), 0x00 }, > > > > - { IMX290_REG_8BIT(0x344e), 0x1f }, > > > > - { IMX290_REG_8BIT(0x344f), 0x00 }, > > > > - { IMX290_REG_8BIT(0x3450), 0x77 }, > > > > - { IMX290_REG_8BIT(0x3451), 0x00 }, > > > > - { IMX290_REG_8BIT(0x3452), 0x1f }, > > > > - { IMX290_REG_8BIT(0x3453), 0x00 }, > > > > - { IMX290_REG_8BIT(0x3454), 0x17 }, > > > > - { IMX290_REG_8BIT(0x3455), 0x00 }, > > > > + { IMX290_REPETITION, 0x10 }, > > > > + { IMX290_TCLKPOST, 87 }, > > > > + { IMX290_THSZERO, 55 }, > > > > + { IMX290_THSPREPARE, 31 }, > > > > + { IMX290_TCLKTRAIL, 31 }, > > > > + { IMX290_THSTRAIL, 31 }, > > > > + { IMX290_TCLKZERO, 119 }, > > > > + { IMX290_TCLKPREPARE, 31 }, > > > > + { IMX290_TLPX, 23 }, > > > > }; > > > > > > > > static const struct imx290_regval imx290_720p_settings[] = { > > > > /* mode settings */ > > > > - { IMX290_REG_8BIT(0x3007), 0x10 }, > > > > - { IMX290_REG_8BIT(0x303a), 0x06 }, > > > > - { IMX290_REG_8BIT(0x3414), 0x04 }, > > > > - { IMX290_REG_8BIT(0x3472), 0x00 }, > > > > - { IMX290_REG_8BIT(0x3473), 0x05 }, > > > > - { IMX290_REG_8BIT(0x3418), 0xd0 }, > > > > - { IMX290_REG_8BIT(0x3419), 0x02 }, > > > > + { IMX290_CTRL_07, IMX290_WINMODE_720P }, > > > > + { IMX290_WINWV_OB, 6 }, > > > > + { IMX290_OPB_SIZE_V, 4 }, > > > > + { IMX290_X_OUT_SIZE, 1280 }, > > > > + { IMX290_Y_OUT_SIZE, 720 }, > > > > { IMX290_REG_8BIT(0x3012), 0x64 }, > > > > { IMX290_REG_8BIT(0x3013), 0x00 }, > > > > - { IMX290_REG_8BIT(0x305c), 0x20 }, > > > > - { IMX290_REG_8BIT(0x305d), 0x00 }, > > > > - { IMX290_REG_8BIT(0x305e), 0x20 }, > > > > - { IMX290_REG_8BIT(0x305f), 0x01 }, > > > > - { IMX290_REG_8BIT(0x315e), 0x1a }, > > > > - { IMX290_REG_8BIT(0x3164), 0x1a }, > > > > + { IMX290_INCKSEL1, 0x20 }, > > > > + { IMX290_INCKSEL2, 0x00 }, > > > > + { IMX290_INCKSEL3, 0x20 }, > > > > + { IMX290_INCKSEL4, 0x01 }, > > > > + { IMX290_INCKSEL5, 0x1a }, > > > > + { IMX290_INCKSEL6, 0x1a }, > > > > { IMX290_REG_8BIT(0x3480), 0x49 }, > > > > /* data rate settings */ > > > > - { IMX290_REG_8BIT(0x3405), 0x10 }, > > > > - { IMX290_REG_8BIT(0x3446), 0x4f }, > > > > - { IMX290_REG_8BIT(0x3447), 0x00 }, > > > > - { IMX290_REG_8BIT(0x3448), 0x2f }, > > > > - { IMX290_REG_8BIT(0x3449), 0x00 }, > > > > - { IMX290_REG_8BIT(0x344a), 0x17 }, > > > > - { IMX290_REG_8BIT(0x344b), 0x00 }, > > > > - { IMX290_REG_8BIT(0x344c), 0x17 }, > > > > - { IMX290_REG_8BIT(0x344d), 0x00 }, > > > > - { IMX290_REG_8BIT(0x344e), 0x17 }, > > > > - { IMX290_REG_8BIT(0x344f), 0x00 }, > > > > - { IMX290_REG_8BIT(0x3450), 0x57 }, > > > > - { IMX290_REG_8BIT(0x3451), 0x00 }, > > > > - { IMX290_REG_8BIT(0x3452), 0x17 }, > > > > - { IMX290_REG_8BIT(0x3453), 0x00 }, > > > > - { IMX290_REG_8BIT(0x3454), 0x17 }, > > > > - { IMX290_REG_8BIT(0x3455), 0x00 }, > > > > + { IMX290_REPETITION, 0x10 }, > > > > + { IMX290_TCLKPOST, 79 }, > > > > + { IMX290_THSZERO, 47 }, > > > > + { IMX290_THSPREPARE, 23 }, > > > > + { IMX290_TCLKTRAIL, 23 }, > > > > + { IMX290_THSTRAIL, 23 }, > > > > + { IMX290_TCLKZERO, 87 }, > > > > + { IMX290_TCLKPREPARE, 23 }, > > > > + { IMX290_TLPX, 23 }, > > > > }; > > > > > > > > static const struct imx290_regval imx290_10bit_settings[] = { > > > > - { IMX290_REG_8BIT(0x3005), 0x00}, > > > > - { IMX290_REG_8BIT(0x3046), 0x00}, > > > > - { IMX290_REG_8BIT(0x3129), 0x1d}, > > > > - { IMX290_REG_8BIT(0x317c), 0x12}, > > > > - { IMX290_REG_8BIT(0x31ec), 0x37}, > > > > - { IMX290_REG_8BIT(0x3441), 0x0a}, > > > > - { IMX290_REG_8BIT(0x3442), 0x0a}, > > > > - { IMX290_REG_8BIT(0x300a), 0x3c}, > > > > - { IMX290_REG_8BIT(0x300b), 0x00}, > > > > + { IMX290_ADBIT, IMX290_ADBIT_10BIT }, > > > > + { IMX290_OUT_CTRL, IMX290_ODBIT_10BIT }, > > > > + { IMX290_ADBIT1, IMX290_ADBIT1_10BIT }, > > > > + { IMX290_ADBIT2, IMX290_ADBIT2_10BIT }, > > > > + { IMX290_ADBIT3, IMX290_ADBIT3_10BIT }, > > > > + { IMX290_CSI_DT_FMT, IMX290_CSI_DT_FMT_RAW10 }, > > > > + { IMX290_BLKLEVEL, 60 }, > > > > }; > > > > > > > > static const struct imx290_regval imx290_12bit_settings[] = { > > > > - { IMX290_REG_8BIT(0x3005), 0x01 }, > > > > - { IMX290_REG_8BIT(0x3046), 0x01 }, > > > > - { IMX290_REG_8BIT(0x3129), 0x00 }, > > > > - { IMX290_REG_8BIT(0x317c), 0x00 }, > > > > - { IMX290_REG_8BIT(0x31ec), 0x0e }, > > > > - { IMX290_REG_8BIT(0x3441), 0x0c }, > > > > - { IMX290_REG_8BIT(0x3442), 0x0c }, > > > > - { IMX290_REG_8BIT(0x300a), 0xf0 }, > > > > - { IMX290_REG_8BIT(0x300b), 0x00 }, > > > > + { IMX290_ADBIT, IMX290_ADBIT_12BIT }, > > > > + { IMX290_OUT_CTRL, IMX290_ODBIT_12BIT }, > > > > + { IMX290_ADBIT1, IMX290_ADBIT1_12BIT }, > > > > + { IMX290_ADBIT2, IMX290_ADBIT2_12BIT }, > > > > + { IMX290_ADBIT3, IMX290_ADBIT3_12BIT }, > > > > + { IMX290_CSI_DT_FMT, IMX290_CSI_DT_FMT_RAW12 }, > > > > + { IMX290_BLKLEVEL, 240 }, > > > > }; > > > > > > > > /* supported link frequencies */
Hi Alexander, CC'ing Wolfram. On Tue, Aug 23, 2022 at 09:19:36AM +0200, Alexander Stein wrote: > Am Dienstag, 23. August 2022, 04:51:20 CEST schrieb Laurent Pinchart: > > On Tue, Aug 23, 2022 at 04:08:20AM +0300, Laurent Pinchart wrote: > > > On Mon, Jul 25, 2022 at 08:49:40AM +0200, Alexander Stein wrote: > > > > Am Sonntag, 24. Juli 2022, 01:06:29 CEST schrieb Laurent Pinchart: > > > > > On Fri, Jul 22, 2022 at 05:37:53PM +0300, Sakari Ailus wrote: > > > > > > On Thu, Jul 21, 2022 at 01:43:54PM +0200, Alexander Stein wrote: > > > > > > ... > > > > > > > > > > > > > Nice the following snippet does the trick already: > > > > > > > ---8<--- > > > > > > > --- a/drivers/media/i2c/imx290.c > > > > > > > +++ b/drivers/media/i2c/imx290.c > > > > > > > @@ -221,6 +221,7 @@ static const struct imx290_pixfmt imx290_formats[] = > > > > > > > { > > > > > > > static const struct regmap_config imx290_regmap_config = { > > > > > > > .reg_bits = 16, > > > > > > > .val_bits = 8, > > > > > > > + .use_single_read = true, > > > > > > > }; > > > > > > > > > > > > > > static const char * const imx290_test_pattern_menu[] = { > > > > > > > ---8<--- > > > > > > > > > > > > > > As this affects the VC OV9281 as well, any suggestions for a common > > > > > > > property? > > > > > > > > > > > > If there's a 1:1 I²C mux in there between the host and the sensor, should > > > > > > it be in DT as well? I'm not entirely certain it's necessary. > > > > > > > > > > The microcontroller also the sensor clock and power supplies, so it has > > > > > to be modelled in DT in any case. I was trying to avoid exposing it as > > > > > an I2C mux, but maybe we'll have to bite the bullet... > > > > > > > > What is the benefit about exposing a I2C mux? The needed regmap config > > > > option is configured completely independent to this. > > > > > > If the I2C mux in the camera module messes up I2C transfers, the related > > > quirks need to be handled somewhere, and a specific mux driver device in > > > DT could be a good place to report that. There may be other options > > > though. > > From a logical point of view, a i2c mux seems to be correct, but in the end > this quirk is handled by regmap which parses device specific properties. > Adding a (mux) bus property which is applied to all devices seems to be a > hassle, IMHO. > Taking Sakari's suggestion of 'single-octet-read' property where in the DT > bindings this should be added? Wolfram, any opinion on this ? More context is available earlier in this mail thread, but tl;dr, the camera module vendor has interposed a microcontroller between the host and the camera sensor on the I2C bus, and it messes up I2C reads by breaking auto-increment (it also disallows reading everything but a small set of white-listed registers). Writes go through without a problem. > > > > > I've implement support for two camera modules from Vision Components but > > > > > haven't submitted patches yet. See [1] and [2] for DT examples and [3] > > > > > for the driver that handles the microcontroller. > > > > > > > > > > Note that one purpose of the microcontroller is to configure the sensor > > > > > automatically. It can be queried through I2C for a list of supported > > > > > modes, and it can also reconfigure the sensor fully when a mode is > > > > > selected. This is meant to enable development of a single driver that > > > > > will cover all modules, regardless of which camera sensor it integrates. > > > > > I'm not sure what words you will use to voice your opinion on this > > > > > design, but I think I already agree :-) > > > > > > > > > > [1] https://gitlab.com/ideasonboard/nxp/linux/-/blob/pinchartl/v5.19/dev/isp/next/arch/arm64/boot/dts/freescale/imx8mp-maivin-csi1-imx296.dts > > > > > [2] https://gitlab.com/ideasonboard/nxp/linux/-/blob/pinchartl/v5.19/dev/isp/next/arch/arm64/boot/dts/freescale/imx8mp-maivin-csi1-imx327.dts > > > > > [3] https://gitlab.com/ideasonboard/nxp/linux/-/blob/pinchartl/v5.19/dev/isp/next/drivers/media/i2c/vc-mipi.c > > > > > > > > > > > The property could be called e.g. "single-octet-read". I think this should > > > > > > probably be documented in I²C bindings (or even regmap). > > > > > > > > > > I like the idea of making it a DT property global to all I2C devices. It > > > > > should ideally be parsed by the I2C core or by regmap. > > > > > > > > I agree with adding this as a regmap option, like 'big-endian' & > > > > friends, but not so much for I2C core. IMHO the core should only be > > > > interested in handling messages and transfers. Setting up those > > > > correctly is a matter for drivers (which in turn use regmap). > > > > > > I don't want to polute a large number of sensor drivers because of > > > questionable design decisions of a particular module vendor. This type > > > of quirk needs to be handled outside of the sensor driver. > > > > Given that the chip ID is only read to print it to the kernel log, and > > that an incorrectly read ID will not prevent the driver from probing or > > affect its behaviour in any way, would you object to merging this patch, > > with the single read issue to support the Vision Components module being > > handled later ? > > No objection here. This problem is and should stay outside of the sensor > driver. VC platform integration is an additional step.
Hi Dave, On Mon, Oct 10, 2022 at 11:31:33AM +0100, Dave Stevenson wrote: > Hi Laurent > > Do you have plans for a v2 on this patch set? I also have a number of > patches for imx290 and there's little point in causing grief to each > other with conflicts. > Or I could take the non-controversial patches from your set and send a > combined patch set? I'm working on a v2, I'll send it shortly. Do I assume correctly you'll submit IMX327 support on top ? :-) > On Tue, 23 Aug 2022 at 02:12, Laurent Pinchart wrote: > > > > Hi Sakari, > > > > Could you already pick up patches 01/19, 02/19, 04/19, 05/19 and 06/19 > > in your tree ? Your opinion on 03/19 woud be appreciated too, I think > > it's a candidate for merge as well. > > > > On Thu, Jul 21, 2022 at 11:35:21AM +0300, Laurent Pinchart wrote: > > > Hello, > > > > > > This patch series gathers miscellaneous improvements for the imx290 > > > driver. The most notable changes on the kernel side is patch 07/19 that > > > simplifies register access, and on the userspace API side patches 14/19, > > > 15/19 and 18/19 that extend the driver with controls and selection > > > rectangles required by libcamera. > > > > > > Laurent Pinchart (19): > > > media: i2c: imx290: Use device lock for the control handler > > > media: i2c: imx290: Print error code when I2C transfer fails > > > media: i2c: imx290: Specify HMAX values in decimal > > > media: i2c: imx290: Replace macro with explicit ARRAY_SIZE() > > > media: i2c: imx290: Drop imx290_write_buffered_reg() > > > media: i2c: imx290: Drop regmap cache > > > media: i2c: imx290: Support variable-sized registers > > > media: i2c: imx290: Correct register sizes > > > media: i2c: imx290: Simplify error handling when writing registers > > > media: i2c: imx290: Define more register macros > > > media: i2c: imx290: Add exposure time control > > > media: i2c: imx290: Fix max gain value > > > media: i2c: imx290: Split control initialization to separate function > > > media: i2c: imx290: Implement HBLANK and VBLANK controls > > > media: i2c: imx290: Create controls for fwnode properties > > > media: i2c: imx290: Move registers with fixed value to init array > > > media: i2c: imx290: Factor out format retrieval to separate function > > > media: i2c: imx290: Add crop selection targets support > > > media: i2c: imx290: Replace GAIN control with ANALOGUE_GAIN > > > > > > drivers/media/i2c/imx290.c | 781 ++++++++++++++++++++++--------------- > > > 1 file changed, 458 insertions(+), 323 deletions(-)
Hello, On Thu, Jul 21, 2022 at 05:37:23PM +0100, Dave Stevenson wrote: > On Thu, 21 Jul 2022 at 12:32, Alexander Stein wrote: > > Am Donnerstag, 21. Juli 2022, 13:17:21 CEST schrieb Laurent Pinchart: > > > On Thu, Jul 21, 2022 at 12:05:46PM +0200, Alexander Stein wrote: > > > > Am Donnerstag, 21. Juli 2022, 10:35:35 CEST schrieb Laurent Pinchart: > > > > > Add support for the V4L2_CID_HBLANK and V4L2_CID_VBLANK controls to the > > > > > imx290 driver. Make the controls read-only to start with, to report the > > > > > values to userspace for timing calculation. > > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > --- > > > > > > > > > > drivers/media/i2c/imx290.c | 39 +++++++++++++++++++++++++++++++++++++- > > > > > 1 file changed, 38 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > > > > > index 4408dd3e191f..7190399f4111 100644 > > > > > --- a/drivers/media/i2c/imx290.c > > > > > +++ b/drivers/media/i2c/imx290.c > > > > > @@ -146,6 +146,8 @@ struct imx290 { > > > > > struct v4l2_ctrl_handler ctrls; > > > > > struct v4l2_ctrl *link_freq; > > > > > struct v4l2_ctrl *pixel_rate; > > > > > + struct v4l2_ctrl *hblank; > > > > > + struct v4l2_ctrl *vblank; > > > > > struct mutex lock; > > > > > }; > > > > > > > > > > @@ -642,6 +644,20 @@ static int imx290_set_fmt(struct v4l2_subdev *sd, > > > > > if (imx290->pixel_rate) > > > > > __v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate, > > > > > imx290_calc_pixel_rate(imx290)); > > > > > + > > > > > + if (imx290->hblank) { > > > > > + unsigned int hblank = mode->hmax - mode->width; > > > > > + > > > > > + __v4l2_ctrl_modify_range(imx290->hblank, hblank, hblank, > > > > > + 1, hblank); > > > > > + } > > > > > + > > > > > + if (imx290->vblank) { > > > > > + unsigned int vblank = IMX290_VMAX_DEFAULT - mode->height; > > > > > + > > > > > + __v4l2_ctrl_modify_range(imx290->vblank, vblank, vblank, > > > > > + 1, vblank); > > > > > + } > > > > > } > > > > > > > > > > *format = fmt->format; > > > > > @@ -880,9 +896,10 @@ static const struct media_entity_operations imx290_subdev_entity_ops = { > > > > > > > > > > static int imx290_ctrl_init(struct imx290 *imx290) > > > > > { > > > > > + unsigned int blank; > > > > > int ret; > > > > > > > > > > - v4l2_ctrl_handler_init(&imx290->ctrls, 5); > > > > > + v4l2_ctrl_handler_init(&imx290->ctrls, 7); > > > > > imx290->ctrls.lock = &imx290->lock; > > > > > > > > > > v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops, > > > > > @@ -910,6 +927,26 @@ static int imx290_ctrl_init(struct imx290 *imx290) > > > > > ARRAY_SIZE(imx290_test_pattern_menu) - 1, > > > > > 0, 0, imx290_test_pattern_menu); > > > > > > > > > > + /* > > > > > + * Horizontal blanking is controlled through the HMAX register, which > > > > > + * contains a line length in INCK clock units. The INCK frequency is > > > > > + * fixed to 74.25 MHz. The HMAX value is currently fixed to 1100, > > > > > + * convert it to a number of pixels based on the nominal pixel rate. > > > > > + */ > > > > > > > > Currently the driver only supports 37.125 MHz, please refer to > > > > imx290_probe. > > > > > > Indeed. Re-reading the comment, I suspect something is wrong, as hmax is > > > not converted to pixels here (and is also not fixed to 1100). I'll drop the comment in v2. > > > The only > > > datasheet I found that is publicly accessed doesn't explain very clearly > > > how the HMAX value should be computed. Based on your experience with IMX > > > sensors, would you be able to shed some light on this ? > > It is pretty much a standard HTS setting based on a pixel rate that is > fixed at 148.5MPix/s. I'm following you for HTS, but not for the fixed pixel rate. Could you elaborate ? > Likewise VMAX is equivalent to a traditional VTS. Yes, that one is easy. > I've been through the same path in > https://github.com/raspberrypi/linux/commits/rpi-5.15.y/drivers/media/i2c/imx290.c > > > Can you share the link to this datasheet you found? https://static6.arrow.com/aropdfconversion/c0c7efde6571c768020a72f59b226308b9669e45/sony_imx290lqr-c_datasheet.pdf > > Sorry, my experience is more like try and error. I don't fully understand this > > as well, but apparently this depends on frame rate select (FRSEL). > > FRSEL is the one difference between IMX327 and IMX290 (and presumably > IMX462 too). IMX290 adds "0" as a valid value for 120/100fps mode. > However there is no need to change FRSEL for standard frame rate > control - you can set it at 0x01 and get a full range of frame rates > through VMAX and HMAX. If you wished to extend that range for slower > rates, you could conditionally set it to 0x2 to double the frame time. > > > > > > + blank = imx290->current_mode->hmax - imx290->current_mode->width; > > > > > + imx290->hblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops, > > > > > + V4L2_CID_HBLANK, blank, blank, 1, > > > > > + blank); > > > > > + if (imx290->hblank) > > > > > + imx290->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > > > > + > > > > > + blank = IMX290_VMAX_DEFAULT - imx290->current_mode->height; > > > > > + imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops, > > > > > + V4L2_CID_VBLANK, blank, blank, 1, > > > > > + blank); > > > > > + if (imx290->vblank) > > > > > + imx290->vblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > > > > + > > > > > > > > > > imx290->sd.ctrl_handler = &imx290->ctrls; > > > > > > > > > > if (imx290->ctrls.error) {
Hi Laurent On Sun, 16 Oct 2022 at 06:37, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Dave, > > On Mon, Oct 10, 2022 at 11:31:33AM +0100, Dave Stevenson wrote: > > Hi Laurent > > > > Do you have plans for a v2 on this patch set? I also have a number of > > patches for imx290 and there's little point in causing grief to each > > other with conflicts. > > Or I could take the non-controversial patches from your set and send a > > combined patch set? > > I'm working on a v2, I'll send it shortly. Do I assume correctly you'll > submit IMX327 support on top ? :-) Thanks - I'll review it tomorrow and sort my patches on top again. This driver is effectively IMX327 - max 1920x1080@60fps in 12 bit. IMX290 adds a 1920x1080@120fps 10bit only mode which isn't currently supported by the driver. I have patches to add 10bit support, but I don't increase the frame rate in them. IMX462 adds that 1920x1080@120fps mode in both 10 and 12 bit, but again I haven't looked at adding support, partly as I don't have a datasheet for that variant. I may see if the change for 120fps 10bit on imx290 works in 12 bit mode for IMX462. For IMX290, 1080p120 needs a link frequency of 445.5MHz on 4 lanes to be supported (2 lanes not permitted), so there will be more link frequency messing required to support it. The basic numbers say that is fast enough for 12bit as well, so there's hope. Dave > > On Tue, 23 Aug 2022 at 02:12, Laurent Pinchart wrote: > > > > > > Hi Sakari, > > > > > > Could you already pick up patches 01/19, 02/19, 04/19, 05/19 and 06/19 > > > in your tree ? Your opinion on 03/19 woud be appreciated too, I think > > > it's a candidate for merge as well. > > > > > > On Thu, Jul 21, 2022 at 11:35:21AM +0300, Laurent Pinchart wrote: > > > > Hello, > > > > > > > > This patch series gathers miscellaneous improvements for the imx290 > > > > driver. The most notable changes on the kernel side is patch 07/19 that > > > > simplifies register access, and on the userspace API side patches 14/19, > > > > 15/19 and 18/19 that extend the driver with controls and selection > > > > rectangles required by libcamera. > > > > > > > > Laurent Pinchart (19): > > > > media: i2c: imx290: Use device lock for the control handler > > > > media: i2c: imx290: Print error code when I2C transfer fails > > > > media: i2c: imx290: Specify HMAX values in decimal > > > > media: i2c: imx290: Replace macro with explicit ARRAY_SIZE() > > > > media: i2c: imx290: Drop imx290_write_buffered_reg() > > > > media: i2c: imx290: Drop regmap cache > > > > media: i2c: imx290: Support variable-sized registers > > > > media: i2c: imx290: Correct register sizes > > > > media: i2c: imx290: Simplify error handling when writing registers > > > > media: i2c: imx290: Define more register macros > > > > media: i2c: imx290: Add exposure time control > > > > media: i2c: imx290: Fix max gain value > > > > media: i2c: imx290: Split control initialization to separate function > > > > media: i2c: imx290: Implement HBLANK and VBLANK controls > > > > media: i2c: imx290: Create controls for fwnode properties > > > > media: i2c: imx290: Move registers with fixed value to init array > > > > media: i2c: imx290: Factor out format retrieval to separate function > > > > media: i2c: imx290: Add crop selection targets support > > > > media: i2c: imx290: Replace GAIN control with ANALOGUE_GAIN > > > > > > > > drivers/media/i2c/imx290.c | 781 ++++++++++++++++++++++--------------- > > > > 1 file changed, 458 insertions(+), 323 deletions(-) > > -- > Regards, > > Laurent Pinchart
Hi Laurent On Sun, 16 Oct 2022 at 08:34, Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > > Hi Laurent > > On Sun, 16 Oct 2022 at 06:37, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi Dave, > > > > On Mon, Oct 10, 2022 at 11:31:33AM +0100, Dave Stevenson wrote: > > > Hi Laurent > > > > > > Do you have plans for a v2 on this patch set? I also have a number of > > > patches for imx290 and there's little point in causing grief to each > > > other with conflicts. > > > Or I could take the non-controversial patches from your set and send a > > > combined patch set? > > > > I'm working on a v2, I'll send it shortly. Do I assume correctly you'll > > submit IMX327 support on top ? :-) > > Thanks - I'll review it tomorrow and sort my patches on top again. I've reworked my patches on top of yours. It gives r/w VBLANK and HBLANK, and corrects PIXEL_RATE. I'm testing on our 6.0 branch, but https://github.com/6by9/linux/commits/linuxtv_imx290/drivers/media/i2c is against the linux-media branch. I've messed something up in the "media: i2c: imx290: Support 60fps in 2 lane operation" patch at present - I'm looking into what has gone wrong, as the earlier versions of that patch worked fine. The branch will get force-pushed once I've fixed it. > This driver is effectively IMX327 - max 1920x1080@60fps in 12 bit. > > IMX290 adds a 1920x1080@120fps 10bit only mode which isn't currently > supported by the driver. I have patches to add 10bit support, but I > don't increase the frame rate in them. > > IMX462 adds that 1920x1080@120fps mode in both 10 and 12 bit, but > again I haven't looked at adding support, partly as I don't have a > datasheet for that variant. I may see if the change for 120fps 10bit > on imx290 works in 12 bit mode for IMX462. > For IMX290, 1080p120 needs a link frequency of 445.5MHz on 4 lanes to > be supported (2 lanes not permitted), so there will be more link > frequency messing required to support it. The basic numbers say that > is fast enough for 12bit as well, so there's hope. I guess seeing as I'm messing with the clock setup, I may as well keep going and look at the 120fps modes. It's a little trickier as the Pi ISP will be on the edge at those rates, but it should be good enough. There is an awkward question with regard link-frequencies. Is there a need to support multiple sets of link-frequency, or do we support any set of 2? ie for imx290, on 4 lanes do we want: - 891Mbit/s for 1080p120 10bit - 445.5Mbit/s for 1080p60 10 or 12 bit - 594Mbit/s for 720p120 10bit - 297Mbit/s for 720p60 10 and 12 bit all to be present in DT? If only 891 and 594 then you're limited to 10 bit images, but otherwise it should be fully functional. The max frame interval would be half that of 445.5 and 297 though, so there are compromises, but who/what then controls the link_frequency to switch between the ranges? I can see another can of worms being opened here! Dave > Dave > > > > On Tue, 23 Aug 2022 at 02:12, Laurent Pinchart wrote: > > > > > > > > Hi Sakari, > > > > > > > > Could you already pick up patches 01/19, 02/19, 04/19, 05/19 and 06/19 > > > > in your tree ? Your opinion on 03/19 woud be appreciated too, I think > > > > it's a candidate for merge as well. > > > > > > > > On Thu, Jul 21, 2022 at 11:35:21AM +0300, Laurent Pinchart wrote: > > > > > Hello, > > > > > > > > > > This patch series gathers miscellaneous improvements for the imx290 > > > > > driver. The most notable changes on the kernel side is patch 07/19 that > > > > > simplifies register access, and on the userspace API side patches 14/19, > > > > > 15/19 and 18/19 that extend the driver with controls and selection > > > > > rectangles required by libcamera. > > > > > > > > > > Laurent Pinchart (19): > > > > > media: i2c: imx290: Use device lock for the control handler > > > > > media: i2c: imx290: Print error code when I2C transfer fails > > > > > media: i2c: imx290: Specify HMAX values in decimal > > > > > media: i2c: imx290: Replace macro with explicit ARRAY_SIZE() > > > > > media: i2c: imx290: Drop imx290_write_buffered_reg() > > > > > media: i2c: imx290: Drop regmap cache > > > > > media: i2c: imx290: Support variable-sized registers > > > > > media: i2c: imx290: Correct register sizes > > > > > media: i2c: imx290: Simplify error handling when writing registers > > > > > media: i2c: imx290: Define more register macros > > > > > media: i2c: imx290: Add exposure time control > > > > > media: i2c: imx290: Fix max gain value > > > > > media: i2c: imx290: Split control initialization to separate function > > > > > media: i2c: imx290: Implement HBLANK and VBLANK controls > > > > > media: i2c: imx290: Create controls for fwnode properties > > > > > media: i2c: imx290: Move registers with fixed value to init array > > > > > media: i2c: imx290: Factor out format retrieval to separate function > > > > > media: i2c: imx290: Add crop selection targets support > > > > > media: i2c: imx290: Replace GAIN control with ANALOGUE_GAIN > > > > > > > > > > drivers/media/i2c/imx290.c | 781 ++++++++++++++++++++++--------------- > > > > > 1 file changed, 458 insertions(+), 323 deletions(-) > > > > -- > > Regards, > > > > Laurent Pinchart
On Mon, 17 Oct 2022 at 19:07, Dave Stevenson <dave.stevenson@raspberrypi.com> wrote: > > Hi Laurent > > On Sun, 16 Oct 2022 at 08:34, Dave Stevenson > <dave.stevenson@raspberrypi.com> wrote: > > > > Hi Laurent > > > > On Sun, 16 Oct 2022 at 06:37, Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > > > > > > Hi Dave, > > > > > > On Mon, Oct 10, 2022 at 11:31:33AM +0100, Dave Stevenson wrote: > > > > Hi Laurent > > > > > > > > Do you have plans for a v2 on this patch set? I also have a number of > > > > patches for imx290 and there's little point in causing grief to each > > > > other with conflicts. > > > > Or I could take the non-controversial patches from your set and send a > > > > combined patch set? > > > > > > I'm working on a v2, I'll send it shortly. Do I assume correctly you'll > > > submit IMX327 support on top ? :-) > > > > Thanks - I'll review it tomorrow and sort my patches on top again. > > I've reworked my patches on top of yours. It gives r/w VBLANK and > HBLANK, and corrects PIXEL_RATE. > I'm testing on our 6.0 branch, but > https://github.com/6by9/linux/commits/linuxtv_imx290/drivers/media/i2c > is against the linux-media branch. > > I've messed something up in the "media: i2c: imx290: Support 60fps in > 2 lane operation" patch at present - I'm looking into what has gone > wrong, as the earlier versions of that patch worked fine. The branch > will get force-pushed once I've fixed it. Sorted and branch updated. 1080p or 720p, up to 60fps, 10 or 12bit, on 2 or 4 lanes all working. > > This driver is effectively IMX327 - max 1920x1080@60fps in 12 bit. > > > > IMX290 adds a 1920x1080@120fps 10bit only mode which isn't currently > > supported by the driver. I have patches to add 10bit support, but I > > don't increase the frame rate in them. > > > > IMX462 adds that 1920x1080@120fps mode in both 10 and 12 bit, but > > again I haven't looked at adding support, partly as I don't have a > > datasheet for that variant. I may see if the change for 120fps 10bit > > on imx290 works in 12 bit mode for IMX462. > > For IMX290, 1080p120 needs a link frequency of 445.5MHz on 4 lanes to > > be supported (2 lanes not permitted), so there will be more link > > frequency messing required to support it. The basic numbers say that > > is fast enough for 12bit as well, so there's hope. > > I guess seeing as I'm messing with the clock setup, I may as well keep > going and look at the 120fps modes. It's a little trickier as the Pi > ISP will be on the edge at those rates, but it should be good enough. I've got 120fps working on an IMX462 as 720p 10 or 12bit, and as 1080p 10bit. 1080p 12 bit goes psychedelic!
Hi Dave, On Mon, Oct 17, 2022 at 07:07:20PM +0100, Dave Stevenson wrote: > Hi Laurent > > On Sun, 16 Oct 2022 at 08:34, Dave Stevenson > <dave.stevenson@raspberrypi.com> wrote: > > > > Hi Laurent > > > > On Sun, 16 Oct 2022 at 06:37, Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > > > > > > Hi Dave, > > > > > > On Mon, Oct 10, 2022 at 11:31:33AM +0100, Dave Stevenson wrote: > > > > Hi Laurent > > > > > > > > Do you have plans for a v2 on this patch set? I also have a number of > > > > patches for imx290 and there's little point in causing grief to each > > > > other with conflicts. > > > > Or I could take the non-controversial patches from your set and send a > > > > combined patch set? > > > > > > I'm working on a v2, I'll send it shortly. Do I assume correctly you'll > > > submit IMX327 support on top ? :-) > > > > Thanks - I'll review it tomorrow and sort my patches on top again. > > I've reworked my patches on top of yours. It gives r/w VBLANK and > HBLANK, and corrects PIXEL_RATE. > I'm testing on our 6.0 branch, but > https://github.com/6by9/linux/commits/linuxtv_imx290/drivers/media/i2c > is against the linux-media branch. > > I've messed something up in the "media: i2c: imx290: Support 60fps in > 2 lane operation" patch at present - I'm looking into what has gone > wrong, as the earlier versions of that patch worked fine. The branch > will get force-pushed once I've fixed it. > > > This driver is effectively IMX327 - max 1920x1080@60fps in 12 bit. > > > > IMX290 adds a 1920x1080@120fps 10bit only mode which isn't currently > > supported by the driver. I have patches to add 10bit support, but I > > don't increase the frame rate in them. > > > > IMX462 adds that 1920x1080@120fps mode in both 10 and 12 bit, but > > again I haven't looked at adding support, partly as I don't have a > > datasheet for that variant. I may see if the change for 120fps 10bit > > on imx290 works in 12 bit mode for IMX462. > > For IMX290, 1080p120 needs a link frequency of 445.5MHz on 4 lanes to > > be supported (2 lanes not permitted), so there will be more link > > frequency messing required to support it. The basic numbers say that > > is fast enough for 12bit as well, so there's hope. > > I guess seeing as I'm messing with the clock setup, I may as well keep > going and look at the 120fps modes. It's a little trickier as the Pi > ISP will be on the edge at those rates, but it should be good enough. > > There is an awkward question with regard link-frequencies. Is there a > need to support multiple sets of link-frequency, or do we support any > set of 2? > ie for imx290, on 4 lanes do we want: > - 891Mbit/s for 1080p120 10bit > - 445.5Mbit/s for 1080p60 10 or 12 bit > - 594Mbit/s for 720p120 10bit > - 297Mbit/s for 720p60 10 and 12 bit > all to be present in DT? > If only 891 and 594 then you're limited to 10 bit images, but > otherwise it should be fully functional. The max frame interval would > be half that of 445.5 and 297 though, so there are compromises, but > who/what then controls the link_frequency to switch between the > ranges? It's up to the user space to set that control in a general case. I guess there are no specific rules on how many you should put to DT, but generally those that are useful should be there. I wonder why 12 bpp output isn't possible at the double frequency. Of course it is possible the sensor's clock tree makes that impossible but it is still unusual. > > I can see another can of worms being opened here! If this is what the sensor does, how else it should be operated?
Hi Sakari Thanks for the response On Wed, 19 Oct 2022 at 11:33, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > Hi Dave, > > On Mon, Oct 17, 2022 at 07:07:20PM +0100, Dave Stevenson wrote: > > Hi Laurent > > > > On Sun, 16 Oct 2022 at 08:34, Dave Stevenson > > <dave.stevenson@raspberrypi.com> wrote: > > > > > > Hi Laurent > > > > > > On Sun, 16 Oct 2022 at 06:37, Laurent Pinchart > > > <laurent.pinchart@ideasonboard.com> wrote: > > > > > > > > Hi Dave, > > > > > > > > On Mon, Oct 10, 2022 at 11:31:33AM +0100, Dave Stevenson wrote: > > > > > Hi Laurent > > > > > > > > > > Do you have plans for a v2 on this patch set? I also have a number of > > > > > patches for imx290 and there's little point in causing grief to each > > > > > other with conflicts. > > > > > Or I could take the non-controversial patches from your set and send a > > > > > combined patch set? > > > > > > > > I'm working on a v2, I'll send it shortly. Do I assume correctly you'll > > > > submit IMX327 support on top ? :-) > > > > > > Thanks - I'll review it tomorrow and sort my patches on top again. > > > > I've reworked my patches on top of yours. It gives r/w VBLANK and > > HBLANK, and corrects PIXEL_RATE. > > I'm testing on our 6.0 branch, but > > https://github.com/6by9/linux/commits/linuxtv_imx290/drivers/media/i2c > > is against the linux-media branch. > > > > I've messed something up in the "media: i2c: imx290: Support 60fps in > > 2 lane operation" patch at present - I'm looking into what has gone > > wrong, as the earlier versions of that patch worked fine. The branch > > will get force-pushed once I've fixed it. > > > > > This driver is effectively IMX327 - max 1920x1080@60fps in 12 bit. > > > > > > IMX290 adds a 1920x1080@120fps 10bit only mode which isn't currently > > > supported by the driver. I have patches to add 10bit support, but I > > > don't increase the frame rate in them. > > > > > > IMX462 adds that 1920x1080@120fps mode in both 10 and 12 bit, but > > > again I haven't looked at adding support, partly as I don't have a > > > datasheet for that variant. I may see if the change for 120fps 10bit > > > on imx290 works in 12 bit mode for IMX462. > > > For IMX290, 1080p120 needs a link frequency of 445.5MHz on 4 lanes to > > > be supported (2 lanes not permitted), so there will be more link > > > frequency messing required to support it. The basic numbers say that > > > is fast enough for 12bit as well, so there's hope. > > > > I guess seeing as I'm messing with the clock setup, I may as well keep > > going and look at the 120fps modes. It's a little trickier as the Pi > > ISP will be on the edge at those rates, but it should be good enough. > > > > There is an awkward question with regard link-frequencies. Is there a > > need to support multiple sets of link-frequency, or do we support any > > set of 2? > > ie for imx290, on 4 lanes do we want: > > - 891Mbit/s for 1080p120 10bit > > - 445.5Mbit/s for 1080p60 10 or 12 bit > > - 594Mbit/s for 720p120 10bit > > - 297Mbit/s for 720p60 10 and 12 bit > > all to be present in DT? > > If only 891 and 594 then you're limited to 10 bit images, but > > otherwise it should be fully functional. The max frame interval would > > be half that of 445.5 and 297 though, so there are compromises, but > > who/what then controls the link_frequency to switch between the > > ranges? > > It's up to the user space to set that control in a general case. I guess > there are no specific rules on how many you should put to DT, but generally > those that are useful should be there. Does the driver have to support multiple sets of link frequencies simultaneously? The way the driver is currently written you have one link freq for 1080p and one for 720p (2/3rds the rate used for 1080p). You could retain using only 2 link frequencies at a time. If DT/ACPI gives us 222.75MHz and 148.5MHz on 4 lanes, or 445.5MHz and 297MHz on 2 lanes, with IMX327, IMX290 or IMX462, then use the current configuration that can do 0.03 to 60fps as RAW10 or RAW12. If DT/ACPI gives us 445.5MHz and 297MHz on 4 lanes and are on an IMX290 (not IMX327), then use a new configuration that can do 0.06 to 120fps as RAW10 only. If DT/ACPI gives us 445.5MHz and 297MHz on 4 lanes and are on an IMX462, then use a new configuration that can do 0.06 to 120fps as RAW10 or RAW12. If the configuration doesn't fall into any of these categories, then it is rejected. Whoever is putting together the DT/ACPI for the device can then choose whether they value the lower minimum frame rate and RAW12, or the higher frame rate but are prepared to sacrifice RAW12. (As we use dtoverlays, we can add overrides so that person is the end user). Trying to cram that lot in so that it can all be used simultaneously will get quite ugly - the register configuration is not quite as simple as one might hope, and you'd be filtering the permitted modes and bit depths all over the place. As I mentioned at the Media Summit in Dublin I've had users wanting IMX462 for astronomy use cases, so halving the max exposure time by dropping the current max 60fps configuration won't be popular. I guess you could incorrectly use an IMX327 compatible string in the DT when using an IMX290/462 to force the behaviour, but that feels even more of a hack. > I wonder why 12 bpp output isn't possible at the double frequency. Of > course it is possible the sensor's clock tree makes that impossible but it > is still unusual. It is a little weird. As noted in the later emails, I have put together settings to get 120fps running, and have tried it on both IMX462 and IMX290. 720p120 RAW12 works fine on both, as do 720p120 and 1080p120 in RAW10. However 1080p120 RAW12 doesn't work on either, so I suspect it is something in the CSI2 block configuration that can't quite hit that data rate without further changes. I'll see if Sony wants to be friendly with datasheets for the IMX462. > > > > I can see another can of worms being opened here! > > If this is what the sensor does, how else it should be operated? As above, link frequency remains read only based on DT or ACPI, and that then restricts the configurations that are possible. I've never seen a good userspace example of using V4L2_CID_LINK_FREQUENCY, so without that it always seems to be a setting that is generally only used by the CSI-2 receiver to adapt appropriately to the data rate. To my mind it falls into the same category as binning and cropping - it's lovely to expose the full feature set, but that is just passing the buck to some heuristics in userspace. Generally the user of the camera doesn't care (they just want their camera to work) so realistically you're looking at libcamera, and it doesn't necessarily have sufficient information about the sensor or use case to make a good decision. Dave > -- > Kind regards, > > Sakari Ailus
Hi Dave, On Wed, Oct 19, 2022 at 12:38:21PM +0100, Dave Stevenson wrote: > Hi Sakari > > Thanks for the response > > On Wed, 19 Oct 2022 at 11:33, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > > > Hi Dave, > > > > On Mon, Oct 17, 2022 at 07:07:20PM +0100, Dave Stevenson wrote: > > > Hi Laurent > > > > > > On Sun, 16 Oct 2022 at 08:34, Dave Stevenson > > > <dave.stevenson@raspberrypi.com> wrote: > > > > > > > > Hi Laurent > > > > > > > > On Sun, 16 Oct 2022 at 06:37, Laurent Pinchart > > > > <laurent.pinchart@ideasonboard.com> wrote: > > > > > > > > > > Hi Dave, > > > > > > > > > > On Mon, Oct 10, 2022 at 11:31:33AM +0100, Dave Stevenson wrote: > > > > > > Hi Laurent > > > > > > > > > > > > Do you have plans for a v2 on this patch set? I also have a number of > > > > > > patches for imx290 and there's little point in causing grief to each > > > > > > other with conflicts. > > > > > > Or I could take the non-controversial patches from your set and send a > > > > > > combined patch set? > > > > > > > > > > I'm working on a v2, I'll send it shortly. Do I assume correctly you'll > > > > > submit IMX327 support on top ? :-) > > > > > > > > Thanks - I'll review it tomorrow and sort my patches on top again. > > > > > > I've reworked my patches on top of yours. It gives r/w VBLANK and > > > HBLANK, and corrects PIXEL_RATE. > > > I'm testing on our 6.0 branch, but > > > https://github.com/6by9/linux/commits/linuxtv_imx290/drivers/media/i2c > > > is against the linux-media branch. > > > > > > I've messed something up in the "media: i2c: imx290: Support 60fps in > > > 2 lane operation" patch at present - I'm looking into what has gone > > > wrong, as the earlier versions of that patch worked fine. The branch > > > will get force-pushed once I've fixed it. > > > > > > > This driver is effectively IMX327 - max 1920x1080@60fps in 12 bit. > > > > > > > > IMX290 adds a 1920x1080@120fps 10bit only mode which isn't currently > > > > supported by the driver. I have patches to add 10bit support, but I > > > > don't increase the frame rate in them. > > > > > > > > IMX462 adds that 1920x1080@120fps mode in both 10 and 12 bit, but > > > > again I haven't looked at adding support, partly as I don't have a > > > > datasheet for that variant. I may see if the change for 120fps 10bit > > > > on imx290 works in 12 bit mode for IMX462. > > > > For IMX290, 1080p120 needs a link frequency of 445.5MHz on 4 lanes to > > > > be supported (2 lanes not permitted), so there will be more link > > > > frequency messing required to support it. The basic numbers say that > > > > is fast enough for 12bit as well, so there's hope. > > > > > > I guess seeing as I'm messing with the clock setup, I may as well keep > > > going and look at the 120fps modes. It's a little trickier as the Pi > > > ISP will be on the edge at those rates, but it should be good enough. > > > > > > There is an awkward question with regard link-frequencies. Is there a > > > need to support multiple sets of link-frequency, or do we support any > > > set of 2? > > > ie for imx290, on 4 lanes do we want: > > > - 891Mbit/s for 1080p120 10bit > > > - 445.5Mbit/s for 1080p60 10 or 12 bit > > > - 594Mbit/s for 720p120 10bit > > > - 297Mbit/s for 720p60 10 and 12 bit > > > all to be present in DT? > > > If only 891 and 594 then you're limited to 10 bit images, but > > > otherwise it should be fully functional. The max frame interval would > > > be half that of 445.5 and 297 though, so there are compromises, but > > > who/what then controls the link_frequency to switch between the > > > ranges? > > > > It's up to the user space to set that control in a general case. I guess > > there are no specific rules on how many you should put to DT, but generally > > those that are useful should be there. > > Does the driver have to support multiple sets of link frequencies > simultaneously? It's up to the driver. A driver may support fewer features than the hardware allows, and with sensors this is commonly the case. So I don't think this would be special somehow. Of course if a driver supports just one and the DT has more, the end result may not be desirable in terms of what actually works. With e.g. CCS the user can choose any link frequency for which there is a valid PLL tree configuration with current settings. Depending on e.g. the bit depth, for instance, some frequencies may be unavailable. > > The way the driver is currently written you have one link freq for > 1080p and one for 720p (2/3rds the rate used for 1080p). You could > retain using only 2 link frequencies at a time. > > If DT/ACPI gives us 222.75MHz and 148.5MHz on 4 lanes, or 445.5MHz and > 297MHz on 2 lanes, with IMX327, IMX290 or IMX462, then use the current > configuration that can do 0.03 to 60fps as RAW10 or RAW12. > If DT/ACPI gives us 445.5MHz and 297MHz on 4 lanes and are on an > IMX290 (not IMX327), then use a new configuration that can do 0.06 to > 120fps as RAW10 only. > If DT/ACPI gives us 445.5MHz and 297MHz on 4 lanes and are on an > IMX462, then use a new configuration that can do 0.06 to 120fps as > RAW10 or RAW12. > If the configuration doesn't fall into any of these categories, then > it is rejected. Seems reasonable. > > Whoever is putting together the DT/ACPI for the device can then choose > whether they value the lower minimum frame rate and RAW12, or the > higher frame rate but are prepared to sacrifice RAW12. (As we use > dtoverlays, we can add overrides so that person is the end user). > Trying to cram that lot in so that it can all be used simultaneously > will get quite ugly - the register configuration is not quite as > simple as one might hope, and you'd be filtering the permitted modes > and bit depths all over the place. > > As I mentioned at the Media Summit in Dublin I've had users wanting > IMX462 for astronomy use cases, so halving the max exposure time by > dropping the current max 60fps configuration won't be popular. I guess > you could incorrectly use an IMX327 compatible string in the DT when > using an IMX290/462 to force the behaviour, but that feels even more > of a hack. > > > I wonder why 12 bpp output isn't possible at the double frequency. Of > > course it is possible the sensor's clock tree makes that impossible but it > > is still unusual. > > It is a little weird. As noted in the later emails, I have put > together settings to get 120fps running, and have tried it on both > IMX462 and IMX290. > > 720p120 RAW12 works fine on both, as do 720p120 and 1080p120 in RAW10. > However 1080p120 RAW12 doesn't work on either, so I suspect it is > something in the CSI2 block configuration that can't quite hit that > data rate without further changes. I'll see if Sony wants to be > friendly with datasheets for the IMX462. The receiver block driver could refuse to streamon if the pixel rate * bpp is too high. But I understand in this case it shouldn't be a limitation. And it doesn't really help the user to find which configurations would work. > > > > > > > I can see another can of worms being opened here! > > > > If this is what the sensor does, how else it should be operated? > > As above, link frequency remains read only based on DT or ACPI, and > that then restricts the configurations that are possible. > > I've never seen a good userspace example of using > V4L2_CID_LINK_FREQUENCY, so without that it always seems to be a > setting that is generally only used by the CSI-2 receiver to adapt > appropriately to the data rate. > To my mind it falls into the same category as binning and cropping - > it's lovely to expose the full feature set, but that is just passing > the buck to some heuristics in userspace. Generally the user of the > camera doesn't care (they just want their camera to work) so > realistically you're looking at libcamera, and it doesn't necessarily > have sufficient information about the sensor or use case to make a > good decision. The CCS driver changes the link frequency based on other configuration (mbus code) if the selected one cannot be used. As this is a board specific configuration, fixing these values for a sensor is not a feasible approach in libcamera.
Hello, On Wed, Oct 19, 2022 at 04:27:13PM +0300, Sakari Ailus wrote: > On Wed, Oct 19, 2022 at 12:38:21PM +0100, Dave Stevenson wrote: > > On Wed, 19 Oct 2022 at 11:33, Sakari Ailus wrote: > > > On Mon, Oct 17, 2022 at 07:07:20PM +0100, Dave Stevenson wrote: > > > > On Sun, 16 Oct 2022 at 08:34, Dave Stevenson wrote: > > > > > On Sun, 16 Oct 2022 at 06:37, Laurent Pinchart wrote: > > > > > > On Mon, Oct 10, 2022 at 11:31:33AM +0100, Dave Stevenson wrote: > > > > > > > Hi Laurent > > > > > > > > > > > > > > Do you have plans for a v2 on this patch set? I also have a number of > > > > > > > patches for imx290 and there's little point in causing grief to each > > > > > > > other with conflicts. > > > > > > > Or I could take the non-controversial patches from your set and send a > > > > > > > combined patch set? > > > > > > > > > > > > I'm working on a v2, I'll send it shortly. Do I assume correctly you'll > > > > > > submit IMX327 support on top ? :-) > > > > > > > > > > Thanks - I'll review it tomorrow and sort my patches on top again. > > > > > > > > I've reworked my patches on top of yours. It gives r/w VBLANK and > > > > HBLANK, and corrects PIXEL_RATE. > > > > I'm testing on our 6.0 branch, but > > > > https://github.com/6by9/linux/commits/linuxtv_imx290/drivers/media/i2c > > > > is against the linux-media branch. > > > > > > > > I've messed something up in the "media: i2c: imx290: Support 60fps in > > > > 2 lane operation" patch at present - I'm looking into what has gone > > > > wrong, as the earlier versions of that patch worked fine. The branch > > > > will get force-pushed once I've fixed it. > > > > > > > > > This driver is effectively IMX327 - max 1920x1080@60fps in 12 bit. > > > > > > > > > > IMX290 adds a 1920x1080@120fps 10bit only mode which isn't currently > > > > > supported by the driver. I have patches to add 10bit support, but I > > > > > don't increase the frame rate in them. > > > > > > > > > > IMX462 adds that 1920x1080@120fps mode in both 10 and 12 bit, but > > > > > again I haven't looked at adding support, partly as I don't have a > > > > > datasheet for that variant. I may see if the change for 120fps 10bit > > > > > on imx290 works in 12 bit mode for IMX462. > > > > > For IMX290, 1080p120 needs a link frequency of 445.5MHz on 4 lanes to > > > > > be supported (2 lanes not permitted), so there will be more link > > > > > frequency messing required to support it. The basic numbers say that > > > > > is fast enough for 12bit as well, so there's hope. > > > > > > > > I guess seeing as I'm messing with the clock setup, I may as well keep > > > > going and look at the 120fps modes. It's a little trickier as the Pi > > > > ISP will be on the edge at those rates, but it should be good enough. > > > > > > > > There is an awkward question with regard link-frequencies. Is there a > > > > need to support multiple sets of link-frequency, or do we support any > > > > set of 2? > > > > ie for imx290, on 4 lanes do we want: > > > > - 891Mbit/s for 1080p120 10bit > > > > - 445.5Mbit/s for 1080p60 10 or 12 bit > > > > - 594Mbit/s for 720p120 10bit > > > > - 297Mbit/s for 720p60 10 and 12 bit > > > > all to be present in DT? > > > > If only 891 and 594 then you're limited to 10 bit images, but > > > > otherwise it should be fully functional. The max frame interval would > > > > be half that of 445.5 and 297 though, so there are compromises, but > > > > who/what then controls the link_frequency to switch between the > > > > ranges? > > > > > > It's up to the user space to set that control in a general case. I guess > > > there are no specific rules on how many you should put to DT, but generally > > > those that are useful should be there. > > > > Does the driver have to support multiple sets of link frequencies > > simultaneously? > > It's up to the driver. A driver may support fewer features than the > hardware allows, and with sensors this is commonly the case. So I don't > think this would be special somehow. Of course if a driver supports just > one and the DT has more, the end result may not be desirable in terms of > what actually works. > > With e.g. CCS the user can choose any link frequency for which there is a > valid PLL tree configuration with current settings. Depending on e.g. the > bit depth, for instance, some frequencies may be unavailable. > > > The way the driver is currently written you have one link freq for > > 1080p and one for 720p (2/3rds the rate used for 1080p). You could > > retain using only 2 link frequencies at a time. > > > > If DT/ACPI gives us 222.75MHz and 148.5MHz on 4 lanes, or 445.5MHz and > > 297MHz on 2 lanes, with IMX327, IMX290 or IMX462, then use the current > > configuration that can do 0.03 to 60fps as RAW10 or RAW12. > > If DT/ACPI gives us 445.5MHz and 297MHz on 4 lanes and are on an > > IMX290 (not IMX327), then use a new configuration that can do 0.06 to > > 120fps as RAW10 only. > > If DT/ACPI gives us 445.5MHz and 297MHz on 4 lanes and are on an > > IMX462, then use a new configuration that can do 0.06 to 120fps as > > RAW10 or RAW12. > > If the configuration doesn't fall into any of these categories, then > > it is rejected. > > Seems reasonable. > > > Whoever is putting together the DT/ACPI for the device can then choose > > whether they value the lower minimum frame rate and RAW12, or the > > higher frame rate but are prepared to sacrifice RAW12. (As we use > > dtoverlays, we can add overrides so that person is the end user). > > Trying to cram that lot in so that it can all be used simultaneously > > will get quite ugly - the register configuration is not quite as > > simple as one might hope, and you'd be filtering the permitted modes > > and bit depths all over the place. > > > > As I mentioned at the Media Summit in Dublin I've had users wanting > > IMX462 for astronomy use cases, so halving the max exposure time by > > dropping the current max 60fps configuration won't be popular. I guess > > you could incorrectly use an IMX327 compatible string in the DT when > > using an IMX290/462 to force the behaviour, but that feels even more > > of a hack. > > > > > I wonder why 12 bpp output isn't possible at the double frequency. Of > > > course it is possible the sensor's clock tree makes that impossible but it > > > is still unusual. > > > > It is a little weird. As noted in the later emails, I have put > > together settings to get 120fps running, and have tried it on both > > IMX462 and IMX290. > > > > 720p120 RAW12 works fine on both, as do 720p120 and 1080p120 in RAW10. > > However 1080p120 RAW12 doesn't work on either, so I suspect it is > > something in the CSI2 block configuration that can't quite hit that > > data rate without further changes. I'll see if Sony wants to be > > friendly with datasheets for the IMX462. > > The receiver block driver could refuse to streamon if the pixel rate * bpp > is too high. But I understand in this case it shouldn't be a limitation. > And it doesn't really help the user to find which configurations would > work. > > > > > I can see another can of worms being opened here! > > > > > > If this is what the sensor does, how else it should be operated? > > > > As above, link frequency remains read only based on DT or ACPI, and > > that then restricts the configurations that are possible. > > > > I've never seen a good userspace example of using > > V4L2_CID_LINK_FREQUENCY, so without that it always seems to be a > > setting that is generally only used by the CSI-2 receiver to adapt > > appropriately to the data rate. > > To my mind it falls into the same category as binning and cropping - > > it's lovely to expose the full feature set, but that is just passing > > the buck to some heuristics in userspace. Generally the user of the > > camera doesn't care (they just want their camera to work) so > > realistically you're looking at libcamera, and it doesn't necessarily > > have sufficient information about the sensor or use case to make a > > good decision. > > The CCS driver changes the link frequency based on other configuration > (mbus code) if the selected one cannot be used. > > As this is a board specific configuration, fixing these values for a sensor > is not a feasible approach in libcamera. I'm with Dave here, I've never seen a good example of how userspace should use the V4L2_CID_LINK_FREQUENCY control, or even of how the frequencies listed in DT should be picked (for sensors that can accommodate a wide range of link frequencies). Sakari, as you've been the one who pushed for control of the link frequency from userspace, could you enlighten us ? :-)