Hi Hans, On Fri, Feb 10, 2023 at 03:43:51PM +0100, Hans de Goede wrote: > Hi, > > On 2/10/23 14:18, Sakari Ailus wrote: > > 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?). > > vim and emacs also both have support for showing function prototypes, > but this is not only about breaking tooling like that. > > My main objection is not that it confuses various tooling, it also confuses > me as a human if I'm trying to figure out what is going on. The kernel's > internal API documentation generally isn't great so I'm used to just look > at a functions implementation as an alternative. These sort of dark-magic > pre-compiler macros make it very hard for me *as a human* to figure out > what is going on. > > So to me personally this level of code-obfuscation just to avoid 6 chars > ", NULL" per function calls is very much not worth the making things > harder to understand level it adds. > > I mean this will even allow mixing the 3 and 4 parameter variants > in a single .c file! That is just very very confusing and anti KISS. > > Who knows maybe iso-c2023 or whatever will give us default function > arguments values? That would be a nice way to do this, the above > not so much IMHO. > > So I won't be adding this per-processor (dark) magic to my patch-set > for this. > > If people really want this they can add this in a follow-up patch-set. Your arguments are entirely reasonable, but I still prefer to have what is a best fit for most sensor drivers. Although this, as well as other fixed size register access helpers, can be added later as needed. The core functionality is what interests me the most now. Even with one function for read and another (or a few, for bulk data?) for write is a huge improvement over the current situation IMO. -- Kind regards, Sakari Ailus
