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