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
