Re: [PATCH v1 2/5] media: i2c: imx492: driver's header

[Petko Manolov <petko.manolov@konsulko.com>]

On 22-11-28 15:30:37, Laurent Pinchart wrote:
> On Mon, Nov 28, 2022 at 03:14:26PM +0200, Petko Manolov wrote:
> > On 22-11-28 12:47:25, Kieran Bingham wrote:
> > > Quoting Petko Manolov (2022-11-25 15:31:17)
> > > > The header.  For the moment only two modes are supported.
> > > > 
> > > > Signed-off-by: Petko Manolov <petko.manolov@konsulko.com>
> > > > ---
> > > >  drivers/media/i2c/imx492.h | 555 +++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 555 insertions(+)
> > > >  create mode 100644 drivers/media/i2c/imx492.h
> > > > 
> > > > diff --git a/drivers/media/i2c/imx492.h b/drivers/media/i2c/imx492.h
> > > > new file mode 100644
> > > > index 000000000000..301fd66c77d5
> > > > --- /dev/null
> > > > +++ b/drivers/media/i2c/imx492.h
> > > > @@ -0,0 +1,555 @@
> > > > +#ifndef        __imx492_h__
> > > > +#define        __imx492_h__
> > > > +
> > > > +struct imx492_reg {
> > > > +       u16 address;
> > > > +       u8 val;
> > > > +};
> > > > +
> > > > +static const struct imx492_reg mode_17to9_regs[] = {
> > > 
> > > Why is this table in the header? Should it be available in multiple locations?
> > 
> > Nope, but there are multiple modes that will eventually be in use and scrolling
> > down a couple of seconds until one gets to the code started to get a bit boring.
> 
> Ideally we should get rid of those tables and use logic to compute register
> values :-) That's a dream only at this point though.

I don't see this happening anytime soon with this particular sensor. :)

> I agree with Kieran that everything could be in the same file, and I also
> agree with you that it's not nice to have a large list of registers at the
> beginning of the driver. I'm thus fine with either option.

I use one register/value pair in a row while the driver is in development,
because it is easy to modify some of them and put a comment at the back.  Once
done with it multiple entries per line will reduce the pain.

At that point most registers that are common for all modes could be put in a
separate array and modify only those that are relevant for a particular
mode/format.  Not quite there yet...

> > > I think it is likely better in the driver itself.
> > 
> > Will put register definitions in the .c file for the time being.
> 
> -- 
> Regards,
> 
> Laurent Pinchart


		Petko