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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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 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 think it is likely better in the driver itself.
> 
> Will put register definitions in the .c file for the time being.

-- 
Regards,

Laurent Pinchart