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@xxxxxxxxxx>
> >>>>>> ---
> >>>>>> 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
--
Regards,
Laurent Pinchart
