Re: [PATCH 28/57] media: Add ovxxxx_16bit_addr_reg_helpers.h

Laurent Pinchart
2023-02-08T15:40:04Z

On Wed, Feb 08, 2023 at 03:33:29PM +0100, Mauro Carvalho Chehab wrote:
> Em Wed, 8 Feb 2023 13:31:17 +0200
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
>
> > Hi Hans,
> >
> > On Mon, Jan 23, 2023 at 01:51:36PM +0100, Hans de Goede wrote:
> > > The following drivers under drivers/media/i2c: ov08x40.c, ov13858.c,
> > > ov13b10.c, ov2680.c, ov2685.c, ov2740.c, ov4689.c, ov5670.c,
> > > ov5675.c, ov5695.c, ov8856.c, ov9282.c and ov9734.c,
> > >
> > > as well as various "atomisp" sensor drivers in drivers/staging, *all*
> > > use register access helpers with the following function prototypes:
> > >
> > > int ovxxxx_read_reg(struct ovxxxx_dev *sensor, u16 reg,
> > > unsigned int len, u32 *val);
> > >
> > > int ovxxxx_write_reg(struct ovxxxx_dev *sensor, u16 reg,
> > > unsigned int len, u32 val);
> > >
> > > To read/write registers on Omnivision OVxxxx image sensors wich expect
> > > a 16 bit register address in big-endian format and which have 1-3 byte
> > > wide registers, in big-endian format (for the higher width registers).
> > >
> > > Add a new ovxxxx_16bit_addr_reg_helpers.h header file with static inline
> > > versions of these register access helpers, so that this code duplication
> > > can be removed.
> >
> > Ideally we'd have helpers for CCI, of which this is a subset. And on top of
> > regmap. I don't object adding these either though.
>
> Well, ideally, when the atomisp-specific sensor ioctls go away, we can
> merge the atomisp-specific sensor drivers for not-yet-uptreamed sensors
> or modify the existing sensor drivers to accept the atomisp resolutions [1].

Please don't, that's not the right way to handle that. If the ISP needs
extra lines or columns, then the corresponding sensor drivers should be
converted to implement the selection API correctly, instead of adding
"modes".

> So, for now, I wouldn't convert those to use regmap. This can be done
> later with the remaining drivers.
>
> [1] atomisp usually requires 16 extra lines/columns for it to work, so
> the current sensor drivers there allow setting resolutions like
> 1616x1216 at the sensor, while offering 1600x1200 resolution to
> userspace.

--
Regards,

Laurent Pinchart