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