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@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
