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

Laurent Pinchart
2023-02-10T11:35:05Z

Hi Sakari,

On Fri, Feb 10, 2023 at 01:18:12PM +0200, Sakari Ailus wrote:
> On Fri, Feb 10, 2023 at 01:04:48PM +0200, Laurent Pinchart wrote:
> > > > > > > > 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.
> >
> > Why not ? I don't see anything wrong with having that as a single API,
> > it doesn't make life more complicated for driver authors or reviewers.
>
> Given that the reviewers (at least me) haven't had noteworthy issues when
> each driver implements their own register access functions, I'm not
> concerned having ~ six register read functions instead of one or two.
> Driver authors should pick the one that fits the purpose best, and not be
> required to implement wrappers in drivers --- which is exactly the
> situation we have today.

It's of course always technically possibly to have more functions, but I
don't see any practical advantage.

--
Regards,

Laurent Pinchart