| Message ID | 20230123125205.622152-29-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Commit | 65b3974173a7ffede971456de064cec3e9368135 |
| Headers | show |
| Series | media: atomisp: Big power-management changes + lots of fixes | expand |
On Wed, Feb 8, 2023 at 11:52 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Mon, Jan 23, 2023 at 01:51:36PM +0100, Hans de Goede wrote: ... > > 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 ? Also, may I > suggest to have a look at drivers/media/i2c/imx290.c While this is a bit error prone example, a patch is on its way, ... > 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 ? ...with regmap fields I believe you can avoid even that and use proper regmap accessors directly.
Hi Hans, 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. Ideally we'd have helpers for CCI, of which this is a subset. And on top of regmap. I don't object adding these either though.
On Wed, Feb 08, 2023 at 01:27:37PM +0200, Andy Shevchenko wrote: > On Wed, Feb 8, 2023 at 11:52 AM Laurent Pinchart wrote: > > On Mon, Jan 23, 2023 at 01:51:36PM +0100, Hans de Goede wrote: > > ... > > > > 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 ? Also, may I > > suggest to have a look at drivers/media/i2c/imx290.c > > While this is a bit error prone example, a patch is on its way, ... The two cleanups are nice, but they're cleanup, not error fixes :-) > > 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 ? > > ...with regmap fields I believe you can avoid even that and use proper > regmap accessors directly. I haven't looked at regmap fields so I don't know if they're the right tool for the job. If we can use the regmap API as-is without any wrapper, even better. Otherwise, new regmap helpers and/or I2C helpers may also be an option. This is a very common use case, not limited to OV camera sensors, or even camera sensors in general.
On Wed, Feb 8, 2023 at 5:41 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Wed, Feb 08, 2023 at 01:27:37PM +0200, Andy Shevchenko wrote: > > On Wed, Feb 8, 2023 at 11:52 AM Laurent Pinchart wrote: > > > On Mon, Jan 23, 2023 at 01:51:36PM +0100, Hans de Goede wrote: ... > > > > 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 ? Also, may I > > > suggest to have a look at drivers/media/i2c/imx290.c > > > > While this is a bit error prone example, a patch is on its way, ... > > The two cleanups are nice, but they're cleanup, not error fixes :-) It depends on which side you look at it. I admit I haven't dug into the code to see if endianess can be an issue there, but the code itself is not written well, esp. when one offers it as an example. So definitely there is a fix on the upper layer.
On Wed, Feb 8, 2023 at 6:04 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Wed, Feb 08, 2023 at 05:50:26PM +0200, Andy Shevchenko wrote: > > On Wed, Feb 8, 2023 at 5:41 PM Laurent Pinchart wrote: > > > On Wed, Feb 08, 2023 at 01:27:37PM +0200, Andy Shevchenko wrote: > > > > On Wed, Feb 8, 2023 at 11:52 AM Laurent Pinchart wrote: > > > > > On Mon, Jan 23, 2023 at 01:51:36PM +0100, Hans de Goede wrote: ... > > > > > > 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 ? Also, may I > > > > > suggest to have a look at drivers/media/i2c/imx290.c > > > > > > > > While this is a bit error prone example, a patch is on its way, ... > > > > > > The two cleanups are nice, but they're cleanup, not error fixes :-) > > > > It depends on which side you look at it. I admit I haven't dug into > > the code to see if endianess can be an issue there, but the code > > itself is not written well, esp. when one offers it as an example. So > > definitely there is a fix on the upper layer. > > Did I miss something ? Your two patches replace a tiny amount of code > with helper functions that don't change any behaviour. It's nicer with > those helpers, no question about that, but "not written well" is a bit > of a stretch and feels quite insulting. Sorry for your feelings, what I meant is the pure educational purposes of the example. When one takes the mentioned driver as an example and uses the code in a slightly different environment the endianess issue may become a real one. That's why we have helpers in kernel to improve robustness against blind copy'n'paste approach. It does not mean your code is broken per se. > Feel free to submit patches that > add new "well-written" helpers.
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. > 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. > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> --- > >> include/media/ovxxxx_16bit_addr_reg_helpers.h | 93 +++++++++++++++++++ > >> 1 file changed, 93 insertions(+) > >> create mode 100644 include/media/ovxxxx_16bit_addr_reg_helpers.h > >> > >> diff --git a/include/media/ovxxxx_16bit_addr_reg_helpers.h b/include/media/ovxxxx_16bit_addr_reg_helpers.h > >> new file mode 100644 > >> index 000000000000..e2ffee3d797a > >> --- /dev/null > >> +++ b/include/media/ovxxxx_16bit_addr_reg_helpers.h > >> @@ -0,0 +1,93 @@ > >> +/* SPDX-License-Identifier: GPL-2.0 */ > >> +/* > >> + * I2C register access helpers for Omnivision OVxxxx image sensors which 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). > >> + * > >> + * Based on the register helpers from drivers/media/i2c/ov2680.c which is: > >> + * Copyright (C) 2018 Linaro Ltd > >> + */ > >> +#ifndef __OVXXXX_16BIT_ADDR_REG_HELPERS_H > >> +#define __OVXXXX_16BIT_ADDR_REG_HELPERS_H > >> + > >> +#include <asm/unaligned.h> > >> +#include <linux/dev_printk.h> > >> +#include <linux/i2c.h> > >> + > >> +static inline int ovxxxx_read_reg(struct i2c_client *client, u16 reg, > >> + unsigned int len, u32 *val) > >> +{ > >> + struct i2c_msg msgs[2]; > >> + u8 addr_buf[2] = { reg >> 8, reg & 0xff }; > >> + u8 data_buf[4] = { 0, }; > >> + int ret; > >> + > >> + if (len > 4) > >> + return -EINVAL; > >> + > >> + msgs[0].addr = client->addr; > >> + msgs[0].flags = 0; > >> + msgs[0].len = ARRAY_SIZE(addr_buf); > >> + msgs[0].buf = addr_buf; > >> + > >> + msgs[1].addr = client->addr; > >> + msgs[1].flags = I2C_M_RD; > >> + msgs[1].len = len; > >> + msgs[1].buf = &data_buf[4 - len]; > >> + > >> + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); > >> + if (ret != ARRAY_SIZE(msgs)) { > >> + dev_err(&client->dev, "read error: reg=0x%4x: %d\n", reg, ret); > >> + return -EIO; > >> + } > >> + > >> + *val = get_unaligned_be32(data_buf); > >> + > >> + return 0; > >> +} > >> + > >> +#define ovxxxx_read_reg8(s, r, v) ovxxxx_read_reg(s, r, 1, v) > >> +#define ovxxxx_read_reg16(s, r, v) ovxxxx_read_reg(s, r, 2, v) > >> +#define ovxxxx_read_reg24(s, r, v) ovxxxx_read_reg(s, r, 3, v) > >> + > >> +static inline int ovxxxx_write_reg(struct i2c_client *client, u16 reg, > >> + unsigned int len, u32 val) > >> +{ > >> + u8 buf[6]; > >> + int ret; > >> + > >> + if (len > 4) > >> + return -EINVAL; > >> + > >> + put_unaligned_be16(reg, buf); > >> + put_unaligned_be32(val << (8 * (4 - len)), buf + 2); > >> + ret = i2c_master_send(client, buf, len + 2); > >> + if (ret != len + 2) { > >> + dev_err(&client->dev, "write error: reg=0x%4x: %d\n", reg, ret); > >> + return -EIO; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +#define ovxxxx_write_reg8(s, r, v) ovxxxx_write_reg(s, r, 1, v) > >> +#define ovxxxx_write_reg16(s, r, v) ovxxxx_write_reg(s, r, 2, v) > >> +#define ovxxxx_write_reg24(s, r, v) ovxxxx_write_reg(s, r, 3, v) > >> + > >> +static inline int ovxxxx_mod_reg(struct i2c_client *client, u16 reg, u8 mask, u8 val) > >> +{ > >> + u32 readval; > >> + int ret; > >> + > >> + ret = ovxxxx_read_reg8(client, reg, &readval); > >> + if (ret < 0) > >> + return ret; > >> + > >> + readval &= ~mask; > >> + val &= mask; > >> + val |= readval; > >> + > >> + return ovxxxx_write_reg8(client, reg, val); > >> +} > >> + > >> +#endif
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.
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.
Hi, On 2/10/23 11:53, Andy Shevchenko wrote: > 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: > > ... > >>>> 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. > > I don't understand why we can't change regmap? regmap has a facility called > regmap bus which we can provide specifically for these types of devices. What's > wrong to see it done? It is fairly easy to layer the few 16 and 24 bit register accesses over a standard regmap with 16 bit reg-address and 8 bit reg-data width using regmap_bulk_write() to still do the write in e.g. a single i2c-transfer. So if we want regmap for underlying physical layer independence, e.g. spi / i2c / i3c. we can just use standard regmap with a cci_write_reg helper on top. I think that would be the most KISS solution here. One thing to also keep in mind is the amount of work necessary to convert existing sensor drivers. Also keeping in mind that it is not just the in tree sensor drivers, but also all out of tree sensor drivers which I have seen use similar constructs. Requiring drivers to have a list / array of structs of all used register addresses + specifying the width per register address is not going to scale very poorly wrt converting all the code out there and I'm afraid that letting regmap somehow deal with the register-width issue is going to require something like this. Regards, Hans
On Fri, Feb 10, 2023 at 12:19:30PM +0100, Hans de Goede wrote: > Hi, > > On 2/10/23 11:53, Andy Shevchenko wrote: > > 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: > > > > ... > > > >>>> 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. > > > > I don't understand why we can't change regmap? regmap has a facility called > > regmap bus which we can provide specifically for these types of devices. What's > > wrong to see it done? > > It is fairly easy to layer the few 16 and 24 bit register accesses over > a standard regmap with 16 bit reg-address and 8 bit reg-data width using > regmap_bulk_write() to still do the write in e.g. a single i2c-transfer. I think we could also use regmap_raw_write(). > So if we want regmap for underlying physical layer independence, e.g. > spi / i2c / i3c. we can just use standard regmap with a > cci_write_reg helper on top. Agreed. We can start experimenting with this, and if somebody has use cases outside of the camera sensor drivers space, we could later move those helpers to regmap. > I think that would be the most KISS solution here. One thing to also keep > in mind is the amount of work necessary to convert existing sensor drivers. > Also keeping in mind that it is not just the in tree sensor drivers, but > also all out of tree sensor drivers which I have seen use similar constructs. If this was the only issue to handle when porting drivers to mainline and upstreaming them, I'd be happy :-) > Requiring drivers to have a list / array of structs of all used register > addresses + specifying the width per register address is not going to scale > very poorly wrt converting all the code out there and I'm afraid that > letting regmap somehow deal with the register-width issue is going to > require something like this. Did you mean "not going to scale very well" ? I'm not sure to understand what you mean here.
Hi, On 2/10/23 12:45, Laurent Pinchart wrote: > Hi Hans, > > On Fri, Feb 10, 2023 at 12:20:36PM +0100, Hans de Goede wrote: >> On 2/9/23 17:11, 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. >>> >>>> 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. >> >> I completely agree. >> >>> 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. >> >> I did not realize this was CCI I agree renaming the helpers makes sense. >> >> I see there still is a lot of discussion going on. > > I haven't seen any disagreement regarding the cci prefix, so let's go > for that. I'd propose cci_read() and cci_write(). > > Sakari, you and I would prefer layering this on top of regmap, while > Andy proposed extending the regmap API. Let's see if we reach an > anonymous agreement on this. > > Regarding the width-specific versions of the helpers, I really think > encoding the size in the register macros is the best option. It makes > life easier for driver authors (only one function to call, no need to > think about the register width to pick the appropriate function in each > call) and reviewers (same reason), without any drawback in my opinion. > > Another feature I'd like in these helpers is improved error handling. In > quite a few sensor drivers I've written, I've implemented the write > function as > > int foo_write(struct foo *foo, u32 reg, u32 val, int *err) > { > ... > int ret; > > if (err && *err) > return *err; > > ret = real_write(...); > if (ret < 0) { > dev_err(...); > if (err) > *err = ret; > } > > return ret; > } > > This allows callers to write > > int ret = 0; > > foo_write(foo, REG_A, 0, &ret); > foo_write(foo, REG_B, 1, &ret); > foo_write(foo, REG_C, 2, &ret); > foo_write(foo, REG_D, 3, &ret); > > return ret; > > which massively simplifies error handling. I'd like the CCI write helper > to implement such a pattern. Interesting, I see that the passing of the err return pointer is optional, so we can still just do a search replace in existing code setting that to just NULL. I like this I agree we should add this. > >> I'll do a follow up series renaming the helpers and converting the >> atomisp ov2680 sensor driver (!) to the new helpers when the current >> discussion about this is done. > > Thank you in advance. > >> And then we can discuss any further details based on v1 of that >> follow up series. >> >> Regards, >> >> Hans >> >> 1) this is already in media-next, but only used by the 1 staging atomisp sensor driver > > That's fine, let's just make sure not to use these new helpers further > before we rename them. Ack. Regards, Hans > >>>>> 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. >>> >>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>> --- >>>>>> include/media/ovxxxx_16bit_addr_reg_helpers.h | 93 +++++++++++++++++++ >>>>>> 1 file changed, 93 insertions(+) >>>>>> create mode 100644 include/media/ovxxxx_16bit_addr_reg_helpers.h >>>>>> >>>>>> diff --git a/include/media/ovxxxx_16bit_addr_reg_helpers.h b/include/media/ovxxxx_16bit_addr_reg_helpers.h >>>>>> new file mode 100644 >>>>>> index 000000000000..e2ffee3d797a >>>>>> --- /dev/null >>>>>> +++ b/include/media/ovxxxx_16bit_addr_reg_helpers.h >>>>>> @@ -0,0 +1,93 @@ >>>>>> +/* SPDX-License-Identifier: GPL-2.0 */ >>>>>> +/* >>>>>> + * I2C register access helpers for Omnivision OVxxxx image sensors which 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). >>>>>> + * >>>>>> + * Based on the register helpers from drivers/media/i2c/ov2680.c which is: >>>>>> + * Copyright (C) 2018 Linaro Ltd >>>>>> + */ >>>>>> +#ifndef __OVXXXX_16BIT_ADDR_REG_HELPERS_H >>>>>> +#define __OVXXXX_16BIT_ADDR_REG_HELPERS_H >>>>>> + >>>>>> +#include <asm/unaligned.h> >>>>>> +#include <linux/dev_printk.h> >>>>>> +#include <linux/i2c.h> >>>>>> + >>>>>> +static inline int ovxxxx_read_reg(struct i2c_client *client, u16 reg, >>>>>> + unsigned int len, u32 *val) >>>>>> +{ >>>>>> + struct i2c_msg msgs[2]; >>>>>> + u8 addr_buf[2] = { reg >> 8, reg & 0xff }; >>>>>> + u8 data_buf[4] = { 0, }; >>>>>> + int ret; >>>>>> + >>>>>> + if (len > 4) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + msgs[0].addr = client->addr; >>>>>> + msgs[0].flags = 0; >>>>>> + msgs[0].len = ARRAY_SIZE(addr_buf); >>>>>> + msgs[0].buf = addr_buf; >>>>>> + >>>>>> + msgs[1].addr = client->addr; >>>>>> + msgs[1].flags = I2C_M_RD; >>>>>> + msgs[1].len = len; >>>>>> + msgs[1].buf = &data_buf[4 - len]; >>>>>> + >>>>>> + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); >>>>>> + if (ret != ARRAY_SIZE(msgs)) { >>>>>> + dev_err(&client->dev, "read error: reg=0x%4x: %d\n", reg, ret); >>>>>> + return -EIO; >>>>>> + } >>>>>> + >>>>>> + *val = get_unaligned_be32(data_buf); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +#define ovxxxx_read_reg8(s, r, v) ovxxxx_read_reg(s, r, 1, v) >>>>>> +#define ovxxxx_read_reg16(s, r, v) ovxxxx_read_reg(s, r, 2, v) >>>>>> +#define ovxxxx_read_reg24(s, r, v) ovxxxx_read_reg(s, r, 3, v) >>>>>> + >>>>>> +static inline int ovxxxx_write_reg(struct i2c_client *client, u16 reg, >>>>>> + unsigned int len, u32 val) >>>>>> +{ >>>>>> + u8 buf[6]; >>>>>> + int ret; >>>>>> + >>>>>> + if (len > 4) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + put_unaligned_be16(reg, buf); >>>>>> + put_unaligned_be32(val << (8 * (4 - len)), buf + 2); >>>>>> + ret = i2c_master_send(client, buf, len + 2); >>>>>> + if (ret != len + 2) { >>>>>> + dev_err(&client->dev, "write error: reg=0x%4x: %d\n", reg, ret); >>>>>> + return -EIO; >>>>>> + } >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +#define ovxxxx_write_reg8(s, r, v) ovxxxx_write_reg(s, r, 1, v) >>>>>> +#define ovxxxx_write_reg16(s, r, v) ovxxxx_write_reg(s, r, 2, v) >>>>>> +#define ovxxxx_write_reg24(s, r, v) ovxxxx_write_reg(s, r, 3, v) >>>>>> + >>>>>> +static inline int ovxxxx_mod_reg(struct i2c_client *client, u16 reg, u8 mask, u8 val) >>>>>> +{ >>>>>> + u32 readval; >>>>>> + int ret; >>>>>> + >>>>>> + ret = ovxxxx_read_reg8(client, reg, &readval); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + readval &= ~mask; >>>>>> + val &= mask; >>>>>> + val |= readval; >>>>>> + >>>>>> + return ovxxxx_write_reg8(client, reg, val); >>>>>> +} >>>>>> + >>>>>> +#endif >
Hi Hans, On Fri, Feb 10, 2023 at 12:56:45PM +0100, Hans de Goede wrote: > On 2/10/23 12:45, Laurent Pinchart wrote: > > On Fri, Feb 10, 2023 at 12:20:36PM +0100, Hans de Goede wrote: > >> On 2/9/23 17:11, 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. > >>> > >>>> 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. > >> > >> I completely agree. > >> > >>> 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. > >> > >> I did not realize this was CCI I agree renaming the helpers makes sense. > >> > >> I see there still is a lot of discussion going on. > > > > I haven't seen any disagreement regarding the cci prefix, so let's go > > for that. I'd propose cci_read() and cci_write(). > > > > Sakari, you and I would prefer layering this on top of regmap, while > > Andy proposed extending the regmap API. Let's see if we reach an > > anonymous agreement on this. > > > > Regarding the width-specific versions of the helpers, I really think > > encoding the size in the register macros is the best option. It makes > > life easier for driver authors (only one function to call, no need to > > think about the register width to pick the appropriate function in each > > call) and reviewers (same reason), without any drawback in my opinion. > > > > Another feature I'd like in these helpers is improved error handling. In > > quite a few sensor drivers I've written, I've implemented the write > > function as > > > > int foo_write(struct foo *foo, u32 reg, u32 val, int *err) > > { > > ... > > int ret; > > > > if (err && *err) > > return *err; > > > > ret = real_write(...); > > if (ret < 0) { > > dev_err(...); > > if (err) > > *err = ret; > > } > > > > return ret; > > } > > > > This allows callers to write > > > > int ret = 0; > > > > foo_write(foo, REG_A, 0, &ret); > > foo_write(foo, REG_B, 1, &ret); > > foo_write(foo, REG_C, 2, &ret); > > foo_write(foo, REG_D, 3, &ret); > > > > return ret; > > > > which massively simplifies error handling. I'd like the CCI write helper > > to implement such a pattern. > > Interesting, I see that the passing of the err return pointer is optional, > so we can still just do a search replace in existing code setting that > to just NULL. And if someone dislikes having to pass NULL for the last argument, we could use some macro magic to accept both the 3 arguments and 4 arguments variants. int __cci_write3(struct cci *cci, u32 reg, u32 val); int __cci_write4(struct cci *cci, u32 reg, u32 val, int *err); #define __cci_write(_1, _2, _3, _4, NAME, ...) NAME #define cci_write(...) __cci_write(__VA_ARGS__, __cci_write4, __cci_write3)(__VA_ARGS__) > I like this I agree we should add this. Glad you like it :-) > >> I'll do a follow up series renaming the helpers and converting the > >> atomisp ov2680 sensor driver (!) to the new helpers when the current > >> discussion about this is done. > > > > Thank you in advance. > > > >> And then we can discuss any further details based on v1 of that > >> follow up series. > >> > >> Regards, > >> > >> Hans > >> > >> 1) this is already in media-next, but only used by the 1 staging atomisp sensor driver > > > > That's fine, let's just make sure not to use these new helpers further > > before we rename them. > > Ack. > > >>>>> 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. > >>> > >>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>>>>> --- > >>>>>> include/media/ovxxxx_16bit_addr_reg_helpers.h | 93 +++++++++++++++++++ > >>>>>> 1 file changed, 93 insertions(+) > >>>>>> create mode 100644 include/media/ovxxxx_16bit_addr_reg_helpers.h > >>>>>> > >>>>>> diff --git a/include/media/ovxxxx_16bit_addr_reg_helpers.h b/include/media/ovxxxx_16bit_addr_reg_helpers.h > >>>>>> new file mode 100644 > >>>>>> index 000000000000..e2ffee3d797a > >>>>>> --- /dev/null > >>>>>> +++ b/include/media/ovxxxx_16bit_addr_reg_helpers.h > >>>>>> @@ -0,0 +1,93 @@ > >>>>>> +/* SPDX-License-Identifier: GPL-2.0 */ > >>>>>> +/* > >>>>>> + * I2C register access helpers for Omnivision OVxxxx image sensors which 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). > >>>>>> + * > >>>>>> + * Based on the register helpers from drivers/media/i2c/ov2680.c which is: > >>>>>> + * Copyright (C) 2018 Linaro Ltd > >>>>>> + */ > >>>>>> +#ifndef __OVXXXX_16BIT_ADDR_REG_HELPERS_H > >>>>>> +#define __OVXXXX_16BIT_ADDR_REG_HELPERS_H > >>>>>> + > >>>>>> +#include <asm/unaligned.h> > >>>>>> +#include <linux/dev_printk.h> > >>>>>> +#include <linux/i2c.h> > >>>>>> + > >>>>>> +static inline int ovxxxx_read_reg(struct i2c_client *client, u16 reg, > >>>>>> + unsigned int len, u32 *val) > >>>>>> +{ > >>>>>> + struct i2c_msg msgs[2]; > >>>>>> + u8 addr_buf[2] = { reg >> 8, reg & 0xff }; > >>>>>> + u8 data_buf[4] = { 0, }; > >>>>>> + int ret; > >>>>>> + > >>>>>> + if (len > 4) > >>>>>> + return -EINVAL; > >>>>>> + > >>>>>> + msgs[0].addr = client->addr; > >>>>>> + msgs[0].flags = 0; > >>>>>> + msgs[0].len = ARRAY_SIZE(addr_buf); > >>>>>> + msgs[0].buf = addr_buf; > >>>>>> + > >>>>>> + msgs[1].addr = client->addr; > >>>>>> + msgs[1].flags = I2C_M_RD; > >>>>>> + msgs[1].len = len; > >>>>>> + msgs[1].buf = &data_buf[4 - len]; > >>>>>> + > >>>>>> + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); > >>>>>> + if (ret != ARRAY_SIZE(msgs)) { > >>>>>> + dev_err(&client->dev, "read error: reg=0x%4x: %d\n", reg, ret); > >>>>>> + return -EIO; > >>>>>> + } > >>>>>> + > >>>>>> + *val = get_unaligned_be32(data_buf); > >>>>>> + > >>>>>> + return 0; > >>>>>> +} > >>>>>> + > >>>>>> +#define ovxxxx_read_reg8(s, r, v) ovxxxx_read_reg(s, r, 1, v) > >>>>>> +#define ovxxxx_read_reg16(s, r, v) ovxxxx_read_reg(s, r, 2, v) > >>>>>> +#define ovxxxx_read_reg24(s, r, v) ovxxxx_read_reg(s, r, 3, v) > >>>>>> + > >>>>>> +static inline int ovxxxx_write_reg(struct i2c_client *client, u16 reg, > >>>>>> + unsigned int len, u32 val) > >>>>>> +{ > >>>>>> + u8 buf[6]; > >>>>>> + int ret; > >>>>>> + > >>>>>> + if (len > 4) > >>>>>> + return -EINVAL; > >>>>>> + > >>>>>> + put_unaligned_be16(reg, buf); > >>>>>> + put_unaligned_be32(val << (8 * (4 - len)), buf + 2); > >>>>>> + ret = i2c_master_send(client, buf, len + 2); > >>>>>> + if (ret != len + 2) { > >>>>>> + dev_err(&client->dev, "write error: reg=0x%4x: %d\n", reg, ret); > >>>>>> + return -EIO; > >>>>>> + } > >>>>>> + > >>>>>> + return 0; > >>>>>> +} > >>>>>> + > >>>>>> +#define ovxxxx_write_reg8(s, r, v) ovxxxx_write_reg(s, r, 1, v) > >>>>>> +#define ovxxxx_write_reg16(s, r, v) ovxxxx_write_reg(s, r, 2, v) > >>>>>> +#define ovxxxx_write_reg24(s, r, v) ovxxxx_write_reg(s, r, 3, v) > >>>>>> + > >>>>>> +static inline int ovxxxx_mod_reg(struct i2c_client *client, u16 reg, u8 mask, u8 val) > >>>>>> +{ > >>>>>> + u32 readval; > >>>>>> + int ret; > >>>>>> + > >>>>>> + ret = ovxxxx_read_reg8(client, reg, &readval); > >>>>>> + if (ret < 0) > >>>>>> + return ret; > >>>>>> + > >>>>>> + readval &= ~mask; > >>>>>> + val &= mask; > >>>>>> + val |= readval; > >>>>>> + > >>>>>> + return ovxxxx_write_reg8(client, reg, val); > >>>>>> +} > >>>>>> + > >>>>>> +#endif
Hi Laurent, On Fri, Feb 10, 2023 at 01:45:10PM +0200, Laurent Pinchart wrote: > Regarding the width-specific versions of the helpers, I really think > encoding the size in the register macros is the best option. It makes > life easier for driver authors (only one function to call, no need to > think about the register width to pick the appropriate function in each > call) and reviewers (same reason), without any drawback in my opinion. As I noted previously, this works well for drivers that need to access registers with multiple widths, which indeed applies to the vast majority of camera sensor drivers, but not to e.g. flash or lens VCM drivers. Fixed width registers are better served with a width-specific function. But these can be always added later on.
Hi, On 2/10/23 13:17, Sakari Ailus wrote: > Hi Laurent, > > On Fri, Feb 10, 2023 at 02:09:02PM +0200, Laurent Pinchart wrote: >> Hi Hans, >> >> On Fri, Feb 10, 2023 at 12:56:45PM +0100, Hans de Goede wrote: >>> On 2/10/23 12:45, Laurent Pinchart wrote: >>>> On Fri, Feb 10, 2023 at 12:20:36PM +0100, Hans de Goede wrote: >>>>> On 2/9/23 17:11, 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. >>>>>> >>>>>>> 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. >>>>> >>>>> I completely agree. >>>>> >>>>>> 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. >>>>> >>>>> I did not realize this was CCI I agree renaming the helpers makes sense. >>>>> >>>>> I see there still is a lot of discussion going on. >>>> >>>> I haven't seen any disagreement regarding the cci prefix, so let's go >>>> for that. I'd propose cci_read() and cci_write(). >>>> >>>> Sakari, you and I would prefer layering this on top of regmap, while >>>> Andy proposed extending the regmap API. Let's see if we reach an >>>> anonymous agreement on this. >>>> >>>> Regarding the width-specific versions of the helpers, I really think >>>> encoding the size in the register macros is the best option. It makes >>>> life easier for driver authors (only one function to call, no need to >>>> think about the register width to pick the appropriate function in each >>>> call) and reviewers (same reason), without any drawback in my opinion. >>>> >>>> Another feature I'd like in these helpers is improved error handling. In >>>> quite a few sensor drivers I've written, I've implemented the write >>>> function as >>>> >>>> int foo_write(struct foo *foo, u32 reg, u32 val, int *err) >>>> { >>>> ... >>>> int ret; >>>> >>>> if (err && *err) >>>> return *err; >>>> >>>> ret = real_write(...); >>>> if (ret < 0) { >>>> dev_err(...); >>>> if (err) >>>> *err = ret; >>>> } >>>> >>>> return ret; >>>> } >>>> >>>> This allows callers to write >>>> >>>> int ret = 0; >>>> >>>> foo_write(foo, REG_A, 0, &ret); >>>> foo_write(foo, REG_B, 1, &ret); >>>> foo_write(foo, REG_C, 2, &ret); >>>> foo_write(foo, REG_D, 3, &ret); >>>> >>>> return ret; >>>> >>>> which massively simplifies error handling. I'd like the CCI write helper >>>> to implement such a pattern. >>> >>> Interesting, I see that the passing of the err return pointer is optional, >>> so we can still just do a search replace in existing code setting that >>> to just NULL. >> >> And if someone dislikes having to pass NULL for the last argument, we >> could use some macro magic to accept both the 3 arguments and 4 >> arguments variants. >> >> int __cci_write3(struct cci *cci, u32 reg, u32 val); >> int __cci_write4(struct cci *cci, u32 reg, u32 val, int *err); >> >> #define __cci_write(_1, _2, _3, _4, NAME, ...) NAME >> #define cci_write(...) __cci_write(__VA_ARGS__, __cci_write4, __cci_write3)(__VA_ARGS__) > > This would be nice, yes. Disagree, see my reply directly to Laurent. > Who will now write the patches for this? :-) I have already more or less volunteered / I opened this can of worms, so that would be me... I see in your other reply that you are fine with going without wrappers for the fixed width accesses for now, good. TBH I don't think these will add much value. I'll try to make some time to work on this somewhere the next couple of weeks. Here is a rough sketch of the API for initial discussion: /* * Note cci_reg_8 deliberately is 0, not 1, so that raw * (not wrapped in a CCI_REG*() macro) register addresses * do 8 bit wide accesses. This allows unchanged use of register * initialization lists of raw address, value pairs which only * do 8 bit width accesses. */ enum cci_reg_type { cci_reg_8 = 0, cci_reg_16, cci_reg_24, cci_reg_32, }; /* * Macros to define register address with the register width encoded * into the higher bits. CCI_REG8() is a no-op so its use is optional. */ #define CCI_REG_SIZE_SHIFT 16 #define CCI_REG_ADDR_MASK GENMASK(15, 0) #define CCI_REG8(x) ((cci_reg_8 << CCI_REG_SIZE_SHIFT) | (x)) #define CCI_REG16(x) ((cci_reg_16 << CCI_REG_SIZE_SHIFT) | (x)) #define CCI_REG24(x) ((cci_reg_24 << CCI_REG_SIZE_SHIFT) | (x)) #define CCI_REG32(x) ((cci_reg_32 << CCI_REG_SIZE_SHIFT) | (x)) int cci_read(struct regmap *regmap, u32 reg, u32 *val, int *err); int cci_write(struct regmap *regmap, u32 reg, u32 val, int *err); int cci_update_bits(struct regmap *map, u32 reg, u32 mask, u32 val, int *err); int cci_multi_reg_write(struct regmap *map, const struct reg_sequence *regs, int num_regs, int *err); Note the regmap here is intended to be a regmap with 16 bit register-address width and 8 bit register-data width. I'll add a helper to get the regmap from an i2c_client to the initial implementation. Also note that all the function names have been chosen to be 1:1 mirrors of the matching regmap functions with the addition of the *err argument. Regards, Hans
Hi Hans, On Fri, Feb 10, 2023 at 01:47:49PM +0100, Hans de Goede wrote: > > And if someone dislikes having to pass NULL for the last argument, we > > could use some macro magic to accept both the 3 arguments and 4 > > arguments variants. > > > > int __cci_write3(struct cci *cci, u32 reg, u32 val); > > int __cci_write4(struct cci *cci, u32 reg, u32 val, int *err); > > > > #define __cci_write(_1, _2, _3, _4, NAME, ...) NAME > > #define cci_write(...) __cci_write(__VA_ARGS__, __cci_write4, __cci_write3)(__VA_ARGS__) > > TBH this just feels like code obfuscation to me and it is also going > to write havoc with various smarted code-editors / IDEs which give > proptype info to the user while typing the function name. > > Having the extra ", NULL" there in calls which don't use / need > the *err thingie really is not a big deal IMHO. It's still an eyesore if the driver doesn't use that pattern of register access error handling. I also prioritise source code itself rather than try to make it fit for a particular editor (which is neither Emacs nor Vim I suppose?). My preference is to provide an interface that best suits a driver's needs, whatever that is. There are just a couple of common patterns and the one above is one of the rare ones.
Hi, On 2/10/23 14:18, Sakari Ailus wrote: > Hi Hans, > > On Fri, Feb 10, 2023 at 01:47:49PM +0100, Hans de Goede wrote: >>> And if someone dislikes having to pass NULL for the last argument, we >>> could use some macro magic to accept both the 3 arguments and 4 >>> arguments variants. >>> >>> int __cci_write3(struct cci *cci, u32 reg, u32 val); >>> int __cci_write4(struct cci *cci, u32 reg, u32 val, int *err); >>> >>> #define __cci_write(_1, _2, _3, _4, NAME, ...) NAME >>> #define cci_write(...) __cci_write(__VA_ARGS__, __cci_write4, __cci_write3)(__VA_ARGS__) >> >> TBH this just feels like code obfuscation to me and it is also going >> to write havoc with various smarted code-editors / IDEs which give >> proptype info to the user while typing the function name. >> >> Having the extra ", NULL" there in calls which don't use / need >> the *err thingie really is not a big deal IMHO. > > It's still an eyesore if the driver doesn't use that pattern of register > access error handling. I also prioritise source code itself rather than try > to make it fit for a particular editor (which is neither Emacs nor Vim I > suppose?). vim and emacs also both have support for showing function prototypes, but this is not only about breaking tooling like that. My main objection is not that it confuses various tooling, it also confuses me as a human if I'm trying to figure out what is going on. The kernel's internal API documentation generally isn't great so I'm used to just look at a functions implementation as an alternative. These sort of dark-magic pre-compiler macros make it very hard for me *as a human* to figure out what is going on. So to me personally this level of code-obfuscation just to avoid 6 chars ", NULL" per function calls is very much not worth the making things harder to understand level it adds. I mean this will even allow mixing the 3 and 4 parameter variants in a single .c file! That is just very very confusing and anti KISS. Who knows maybe iso-c2023 or whatever will give us default function arguments values? That would be a nice way to do this, the above not so much IMHO. So I won't be adding this per-processor (dark) magic to my patch-set for this. If people really want this they can add this in a follow-up patch-set. Regards, Hans
Hi Sakari, On Fri, Feb 10, 2023 at 02:26:31PM +0200, Sakari Ailus wrote: > On Fri, Feb 10, 2023 at 01:45:10PM +0200, Laurent Pinchart wrote: > > Regarding the width-specific versions of the helpers, I really think > > encoding the size in the register macros is the best option. It makes > > life easier for driver authors (only one function to call, no need to > > think about the register width to pick the appropriate function in each > > call) and reviewers (same reason), without any drawback in my opinion. > > As I noted previously, this works well for drivers that need to access > registers with multiple widths, which indeed applies to the vast majority > of camera sensor drivers, but not to e.g. flash or lens VCM drivers. Fixed > width registers are better served with a width-specific function. But these > can be always added later on. I still fail to see why they would be *better* served by custom functions.
Hi Laurent, Andy, On Fri, Feb 10, 2023 at 06:39:11PM +0200, Laurent Pinchart wrote: > On Fri, Feb 10, 2023 at 05:42:25PM +0200, Andy Shevchenko wrote: > > On Fri, Feb 10, 2023 at 02:26:31PM +0200, Sakari Ailus wrote: > > > On Fri, Feb 10, 2023 at 01:45:10PM +0200, Laurent Pinchart wrote: > > > > Regarding the width-specific versions of the helpers, I really think > > > > encoding the size in the register macros is the best option. It makes > > > > life easier for driver authors (only one function to call, no need to > > > > think about the register width to pick the appropriate function in each > > > > call) and reviewers (same reason), without any drawback in my opinion. > > > > > > As I noted previously, this works well for drivers that need to access > > > registers with multiple widths, which indeed applies to the vast majority > > > of camera sensor drivers, but not to e.g. flash or lens VCM drivers. Fixed > > > width registers are better served with a width-specific function. But these > > > can be always added later on. > > > > Again, we can extend regmap to have something like > > > > int (*reg_width)(regmap *, offset) > > > > callback added that will tell the regmap bus underneath what size to use. > > > > In the driver one will define the respective method to return these widths. > > I don't think that's worth it, it would make drivers quite complex > compared to encoding the register width in the register address macros. > We're dealing with devices that have hundreds of registers of various > widths interleaved, a big switch/case for every write isn't great. I'd really prefer the register width information kept as close as possible to the register address, and most probably the best way is to be part of the same macro.
diff --git a/include/media/ovxxxx_16bit_addr_reg_helpers.h b/include/media/ovxxxx_16bit_addr_reg_helpers.h new file mode 100644 index 000000000000..e2ffee3d797a --- /dev/null +++ b/include/media/ovxxxx_16bit_addr_reg_helpers.h @@ -0,0 +1,93 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * I2C register access helpers for Omnivision OVxxxx image sensors which 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). + * + * Based on the register helpers from drivers/media/i2c/ov2680.c which is: + * Copyright (C) 2018 Linaro Ltd + */ +#ifndef __OVXXXX_16BIT_ADDR_REG_HELPERS_H +#define __OVXXXX_16BIT_ADDR_REG_HELPERS_H + +#include <asm/unaligned.h> +#include <linux/dev_printk.h> +#include <linux/i2c.h> + +static inline int ovxxxx_read_reg(struct i2c_client *client, u16 reg, + unsigned int len, u32 *val) +{ + struct i2c_msg msgs[2]; + u8 addr_buf[2] = { reg >> 8, reg & 0xff }; + u8 data_buf[4] = { 0, }; + int ret; + + if (len > 4) + return -EINVAL; + + msgs[0].addr = client->addr; + msgs[0].flags = 0; + msgs[0].len = ARRAY_SIZE(addr_buf); + msgs[0].buf = addr_buf; + + msgs[1].addr = client->addr; + msgs[1].flags = I2C_M_RD; + msgs[1].len = len; + msgs[1].buf = &data_buf[4 - len]; + + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); + if (ret != ARRAY_SIZE(msgs)) { + dev_err(&client->dev, "read error: reg=0x%4x: %d\n", reg, ret); + return -EIO; + } + + *val = get_unaligned_be32(data_buf); + + return 0; +} + +#define ovxxxx_read_reg8(s, r, v) ovxxxx_read_reg(s, r, 1, v) +#define ovxxxx_read_reg16(s, r, v) ovxxxx_read_reg(s, r, 2, v) +#define ovxxxx_read_reg24(s, r, v) ovxxxx_read_reg(s, r, 3, v) + +static inline int ovxxxx_write_reg(struct i2c_client *client, u16 reg, + unsigned int len, u32 val) +{ + u8 buf[6]; + int ret; + + if (len > 4) + return -EINVAL; + + put_unaligned_be16(reg, buf); + put_unaligned_be32(val << (8 * (4 - len)), buf + 2); + ret = i2c_master_send(client, buf, len + 2); + if (ret != len + 2) { + dev_err(&client->dev, "write error: reg=0x%4x: %d\n", reg, ret); + return -EIO; + } + + return 0; +} + +#define ovxxxx_write_reg8(s, r, v) ovxxxx_write_reg(s, r, 1, v) +#define ovxxxx_write_reg16(s, r, v) ovxxxx_write_reg(s, r, 2, v) +#define ovxxxx_write_reg24(s, r, v) ovxxxx_write_reg(s, r, 3, v) + +static inline int ovxxxx_mod_reg(struct i2c_client *client, u16 reg, u8 mask, u8 val) +{ + u32 readval; + int ret; + + ret = ovxxxx_read_reg8(client, reg, &readval); + if (ret < 0) + return ret; + + readval &= ~mask; + val &= mask; + val |= readval; + + return ovxxxx_write_reg8(client, reg, val); +} + +#endif
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. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- include/media/ovxxxx_16bit_addr_reg_helpers.h | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) create mode 100644 include/media/ovxxxx_16bit_addr_reg_helpers.h