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

Sakari Ailus
2023-02-10T10:48:10Z

Hi Laurent,

On Fri, Feb 10, 2023 at 12:29:19PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Fri, Feb 10, 2023 at 12:21:15PM +0200, Sakari Ailus wrote:
> > On Thu, Feb 09, 2023 at 06:11:12PM +0200, Laurent Pinchart wrote:
> > > On Thu, Feb 09, 2023 at 04:03:22PM +0100, Hans de Goede wrote:
> > > > On 2/8/23 10:52, Laurent Pinchart wrote:
> > > > > 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.
> > > > >
> > > > > Any reason to hand-roll those instead of using regmap ?
> > > >
> > > > These devices have a mix of 8 + 16 + 24 bit registers which regmap
> > > > appears to not handle, a regmap has a single regmap_config struct
> > > > with a single "@reg_bits: Number of bits in a register address, mandatory",
> > > > so we would still need wrappers around regmap, at which point it
> > > > really offers us very little.
> > >
> > > We could extend regmap too, although that may be too much yak shaving.
> > > It would be nice, but I won't push hard for it.
> >
> > I took a look at this some time ago, too, and current regmap API is a poor
> > fit for CCI devices. CCI works on top of e.g. both I²C and I3C so something
> > on top of regmap is a better approach indeed.
>
> I'm confused, is regmap a poor fit, or a better approach ?

I'm proposing having something on top of regmap, but not changing regmap
itself.

>
> > Nearly all other devices have a fixed register width, so the regmap API
> > makes sense.
> >
> > > > Also I'm moving duplicate code present in many of the
> > > > drivers/media/i2c/ov*.c files into a common header to remove
> > > > duplicate code. The handrolling was already there before :)
> > > >
> > > > My goal with the new ovxxxx_16bit_addr_reg_helpers.h file was to
> > > > offer something which is as much of a drop-in replacement of the
> > > > current handrolled code as possible (usable with just a few
> > > > search-n-replaces) as possible.
> > > >
> > > > Basically my idea here was to factor out code which I noticed was
> > > > being repeated over and over again. My goal was not to completely
> > > > redo how register accesses are done in these drivers.
> > > >
> > > > I realize I have not yet converted any other drivers, that is because
> > > > I don't really have a way to test most of the other drivers. OTOH
> > > > with the current helpers most conversions should be fairly simply
> > > > and remove a nice amount of code. So maybe I should just only compile
> > > > test the conversions ?
> > >
> > > Before you spend time converting drivers, I'd like to complete the
> > > discussion regarding the design of those helpers. I'd rather avoid
> > > mass-patching drivers now and doing it again in the next kernel release.
> > >
> > > Sakari mentioned CCI (part of the CSI-2 specification). I think that
> > > would be a good name to replace ov* here, as none of this is specific to
> > > OmniVision.
> > >
> > > > > Also, may I
> > > > > suggest to have a look at drivers/media/i2c/imx290.c for an example of
> > > > > how registers of different sizes can be handled in a less error-prone
> > > > > way, using single read/write functions that adapt to the size
> > > > > automatically ?
> > > >
> > > > Yes I have seen this pattern in drivers/media/i2c/ov5693.c too
> > > > (at least I assume it is the same pattern you are talking about).
> > >
> > > Correct. Can we use something like that to merge all the ov*_write_reg()
> > > variants into a single function ? Having to select the size manually in
> > > each call (either by picking the function variant, or by passing a size
> > > as a function parameter) is error-prone. Encoding the size in the
> > > register macro is much safer, easing both development and review.
> >
> > I think so, too.
> >
> > That doesn't mean we shouldn't have function variants for specific register
> > sizes (taking just register addresses) though.
>
> I don't see why we should have multiple APIs when a single one works.

Yes, it "works", but the purpose of the API is to avoid driver code. A
driver accessing fixed width registers is likely to use a helper function
with an API that requires encoding the width into the register address.

--
Regards,

Sakari Ailus