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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent,

On Thu, Feb 09, 2023 at 06:11:12PM +0200, Laurent Pinchart wrote:
> Hi Hans,
> 
> 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.

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.

-- 
Regards,

Sakari Ailus




[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux