Re: Re: Re: [PATCH 07/19] media: i2c: imx290: Support variable-sized registers

[Alexander Stein <alexander.stein@ew.tq-group.com>]

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