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.
--
Regards,
Laurent Pinchart
