| Message ID | 20230123125205.622152-29-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: atomisp: Big power-management changes + lots of fixes | expand |
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. ... > +#include <asm/unaligned.h> Usually we put linux/* followed by asm/*. > +#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 }; Let's use unaligned.h or byteorder/generic.h? __be16 addr_buf = cpu_to_be16(reg); > + u8 data_buf[4] = { 0, }; 0, is not needed. > + 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[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]; This trick I don't like. Can we have like other driver have it, i.e. switch case for the possible length and explicit usage of the endian conversion calls? > + 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); Something similar here? > + 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; Usual pattern: val = (readval & ~mask) | (val & mask); > + return ovxxxx_write_reg8(client, reg, val); > +}
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); > While reviewing the following patch, I realize that we actually don't need these long names. int ov_read_reg(struct ovxxxx_dev *sensor, u16 reg, unsigned int len, u32 *val); int ov_write_reg(struct ovxxxx_dev *sensor, u16 reg, unsigned int len, u32 val); will work fine and fit one line (80 limit).
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. A couple more comments. ... > +#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) Btw, we probably can use _Generic() for these... ... > +static inline int ovxxxx_mod_reg(struct i2c_client *client, u16 reg, u8 mask, u8 val) Can we actually s/mod/update/ as it's more regular when we talk about IO accessor APIs?
Hi, On 1/23/23 19:09, Andy Shevchenko 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. > > ... > >> +#include <asm/unaligned.h> > > Usually we put linux/* followed by asm/*. Ack. > >> +#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 }; > > Let's use unaligned.h or byteorder/generic.h? > > __be16 addr_buf = cpu_to_be16(reg); Ack. > >> + u8 data_buf[4] = { 0, }; > > 0, is not needed. > >> + 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[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]; > > This trick I don't like. Can we have like other driver have it, i.e. switch > case for the possible length and explicit usage of the endian conversion calls? This new header (which is intended to eventually be used in many other ovXXXX drivers too) is modeled after the reg access helpers in drivers/media/i2c/ov*.c And those do use be16 for the addr_buf in some cases, so I'm fine with changing that. But non of them do a switch-case on len, instead they all use similar tricks as this code (which was copied from drivers/media/i2c/ov2680.c) does. So I would prefer to keep this as is, so that the new ovxxxx_16bit_addr_reg_helpers.h code is more like the code which it intends to replace. Regards, Hans > >> + 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); > > Something similar here? > >> + 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; > > Usual pattern: > > val = (readval & ~mask) | (val & mask); > >> + return ovxxxx_write_reg8(client, reg, val); >> +} >
Hi, On 1/23/23 19:23, Andy Shevchenko 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. > > > A couple more comments. > > ... > >> +#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) > > Btw, we probably can use _Generic() for these... > > ... > >> +static inline int ovxxxx_mod_reg(struct i2c_client *client, u16 reg, u8 mask, u8 val) > > Can we actually s/mod/update/ as it's more regular when we talk about IO > accessor APIs? Ack, I'll replace the mod with update. Regards, Hans
On Tue, Jan 24, 2023 at 12:21:50PM +0100, Hans de Goede wrote: > On 1/23/23 19:09, Andy Shevchenko 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. ... > >> + msgs[1].buf = &data_buf[4 - len]; > > > > This trick I don't like. Can we have like other driver have it, i.e. switch > > case for the possible length and explicit usage of the endian conversion > > calls? > > This new header (which is intended to eventually be used in many other > ovXXXX drivers too) is modeled after the reg access helpers > in drivers/media/i2c/ov*.c > > And those do use be16 for the addr_buf in some cases, so I'm fine > with changing that. But non of them do a switch-case on len, > instead they all use similar tricks as this code (which was > copied from drivers/media/i2c/ov2680.c) does. > > So I would prefer to keep this as is, so that the new > ovxxxx_16bit_addr_reg_helpers.h code is more like the code which > it intends to replace. Yes, this is rather for the followup improvements when we have all drivers use these helpers. But under "other drivers" I meant more or less IIO ones where similar (to what I suggest) approach is being used.
Hi Hans, Thank you for the patch. 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 ? 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 ? > 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
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.
Em Wed, 8 Feb 2023 13:31:17 +0200 Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: > 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. Well, ideally, when the atomisp-specific sensor ioctls go away, we can merge the atomisp-specific sensor drivers for not-yet-uptreamed sensors or modify the existing sensor drivers to accept the atomisp resolutions [1]. So, for now, I wouldn't convert those to use regmap. This can be done later with the remaining drivers. [1] atomisp usually requires 16 extra lines/columns for it to work, so the current sensor drivers there allow setting resolutions like 1616x1216 at the sensor, while offering 1600x1200 resolution to userspace. Thanks, Mauro
On Wed, Feb 08, 2023 at 03:33:29PM +0100, Mauro Carvalho Chehab wrote: > Em Wed, 8 Feb 2023 13:31:17 +0200 > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: > > > 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. > > Well, ideally, when the atomisp-specific sensor ioctls go away, we can > merge the atomisp-specific sensor drivers for not-yet-uptreamed sensors > or modify the existing sensor drivers to accept the atomisp resolutions [1]. Please don't, that's not the right way to handle that. If the ISP needs extra lines or columns, then the corresponding sensor drivers should be converted to implement the selection API correctly, instead of adding "modes". > So, for now, I wouldn't convert those to use regmap. This can be done > later with the remaining drivers. > > [1] atomisp usually requires 16 extra lines/columns for it to work, so > the current sensor drivers there allow setting resolutions like > 1616x1216 at the sensor, while offering 1600x1200 resolution to > userspace.
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 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. Feel free to submit patches that add new "well-written" helpers.
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 Andy, On Wed, Feb 08, 2023 at 07:31:43PM +0200, Andy Shevchenko wrote: > On Wed, Feb 8, 2023 at 6:04 PM Laurent Pinchart 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. Reference code should be pristine as much as possible, no disagreement there. That's why I think your two patches are good :-) > > Feel free to submit patches that > > add new "well-written" helpers.
Hi, On 2/8/23 10:52, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > 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. 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 ? > 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). Regards, Hans > >> 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 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 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.
Hi Sakari, 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 ? > 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.
Hi Laurent, On Fri, Feb 10, 2023 at 12:29:19PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > 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. > > > 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.
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?
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.
On Fri, Feb 10, 2023 at 12:53:43PM +0200, 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? How would that work ?
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
Hi Laurent, On 2/9/23 17:11, 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. > >> 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'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. 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 > >>> 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 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.
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 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. > 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. > >>> 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, 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, On 2/10/23 12:35, Laurent Pinchart wrote: > 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 :-) True :) The amount of churn on the stating atomisp sensor drivers which (the few which I have been working on so far) is quite big and that is just inching them closer to being mainline ready. >> 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. Yes my bad I meant to write "not going to scale very well". I think that having to pass these kinda long lists of registers with regmap already when you want to use caching (and need to specify volatile registers which cannot be cached) is a bit of a pain of using regmap (*) Regards, Hans *) Not that I have a better solution for e.g. the volatile registers thing, it just causes a lot of what feels like boilerplate code
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 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. Who will now write the patches for this? :-)
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:09, 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__) 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. 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, 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 Hans, On Fri, Feb 10, 2023 at 01:59:07PM +0100, Hans de Goede wrote: > > 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... :-) Thank you, this is much appreciated, and will allow factoring out ~ 50 lines of code from almost all sensor (as well as a few other) drivers. > > 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. We can get back to this topic when converting drivers. These are convenience wrappers after all and they're easy to add if needed, there's no connection to the underlying implementation which is the important part. > > 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, I'd use capital letters for these as they are macro-like. Or define them as macros. > }; > > /* > * 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)) The top 8 bits could be used for flags in the future, for driver's use. The CCS driver stores register's properties (value format) there. > > 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); We should also have a bulk data write function, although drivers could use regmap directly to do this. Not needed now, nor this is a common need. Just a note. > > 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. CCI also supports 8-bit register addresses (virtually all lens VCM and flash drivers as well as some sensor drivers) so this should be a parameter. I expect some drivers to support CCI via both I²C and I3C. We don't need I3C now as all sensors are I²C, but I²C should be visible in the API only when it's about making the connection to an I²C client. > > 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.
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
On Fri, Feb 10, 2023 at 01:05:52PM +0200, Laurent Pinchart wrote: > On Fri, Feb 10, 2023 at 12:53:43PM +0200, 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? > > How would that work ? If I'm not mistaken, you may introduce something like regmal CCI and then regmap_init_cci(); regmap_read()/regmap_write() regmap_update_bits() regmap_bulk_*() at your service without changing a bit in the drivers (they will use plain regmap APIs instead of custom ones).
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.
Hi Andy, On 2/10/23 16:35, Andy Shevchenko wrote: > On Fri, Feb 10, 2023 at 01:05:52PM +0200, Laurent Pinchart wrote: >> On Fri, Feb 10, 2023 at 12:53:43PM +0200, 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? >> >> How would that work ? > > If I'm not mistaken, you may introduce something like regmal CCI and then > > regmap_init_cci(); > > > regmap_read()/regmap_write() > regmap_update_bits() > regmap_bulk_*() > > at your service without changing a bit in the drivers (they will use plain > regmap APIs instead of custom ones). regmap_bus is for low-level busses like i2c, i3c, spi, etc. We could "abuse" this to overwrite the standard regmap read/write helpers with bus specific ones, but then we loose the actual bus abstraction and we would need separate regmap-cci implementations for i2c/i3c/spi. > 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. That won't work internal helpers to marshall raw-buffers with both reg-addr + reg value(s) to pass to the low-level (i2c/i3c/spi) drivers use an internal regmap.format struct which gets filled with reg-width specific info once from __regmap_init() what you are suggesting really requires major surgery to the whole regmap core. CCI really is an extra protocol layer on top of lower-level / more primitive busses and it is best to have a helper library with a few helpers using regmap underneath to abstract the raw bus accesses away. Note this helper library can be quite thin and small though :) Regards, Hans
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.
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 Hans, On Fri, Feb 10, 2023 at 03:43:51PM +0100, Hans de Goede wrote: > On 2/10/23 14:18, Sakari Ailus wrote: > > 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. The macro-based code I proposed is a poor man's workaround to the lack of both default argument values and function override in plain C. It could be nicer with dedicated language constructs for those, but that won't change how the callers look like (you'll still be able to mix the 3 and 4 parameters variants in a single .c file), only the implementation. > 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. I don't mind much either way personally.
Hi Hans, On Fri, Feb 10, 2023 at 03:43:51PM +0100, Hans de Goede wrote: > 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. Your arguments are entirely reasonable, but I still prefer to have what is a best fit for most sensor drivers. Although this, as well as other fixed size register access helpers, can be added later as needed. The core functionality is what interests me the most now. Even with one function for read and another (or a few, for bulk data?) for write is a huge improvement over the current situation IMO.
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