Re: [PATCH 28/57] media: Add ovxxxx_16bit_addr_reg_helpers.h

Sakari Ailus
2023-02-10T13:18:49Z

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.

--
Regards,

Sakari Ailus