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
