Hi Laurent, 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. -- Regards, Sakari Ailus
