Hi Sakari, On Fri, Feb 10, 2023 at 12:47:55PM +0200, Sakari Ailus wrote: > On Fri, Feb 10, 2023 at 12:29:19PM +0200, Laurent Pinchart wrote: > > 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. Thanks for the clarification. I agree with you. If we later realize that this would make sense within regmap, we can always move the code. > > > 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. 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. -- Regards, Laurent Pinchart
