On Thu, 21 Jul 2016, Masahiro Yamada wrote:
> 2016-07-21 17:41 GMT+09:00 Philipp Zabel <p.za...@pengutronix.de>:
> > Am Donnerstag, den 21.07.2016, 14:05 +0900 schrieb Masahiro Yamada:
> >> The recent update in the reset subsystem requires all reset consumers
> >> to be explicit about the requested reset lines; _explicit or _shared.
> >> This effectively doubled the number of reset_control_get variants.
> >> Also, we already had _optional variants.
> >>
> >> We see some pattern in the reset_control_get APIs.
> >>
> >> There are 6 base functions in terms of function prototype:
> >> reset_control_get
> >> reset_control_get_by_index
> >> of_reset_control_get
> >> of_reset_control_get_by_index
> >> devm_reset_control_get
> >> devm_reset_control_get_by_index
> >>
> >> Each of them has 4 variants with the following suffixes:
> >> _exclusive
> >> _shared
> >> _optional_exclusive
> >> _optional_shared
> >>
> >> The exhaustive set of functions (6 * 4) can be generated with macro
> >> expansion. It will mitigate the pain of maintaining proliferating
> >> APIs.
> >>
> >> I merged similar comment blocks into two, for functions in core.c
> >> because copy-paste work for similar comment blocks is error-prone.
> >>
> >> Signed-off-by: Masahiro Yamada <yamada.masah...@socionext.com>
[...]
I did entertain several ways of reducing this API myself, so I know
where you are coming from but ...
> > I think when Lee suggested the API change, I initially leaned towards a
> > single entry point with flags, similar to the irq request API:
> > reset_control_get(..., RSTF_EXCLUSIVE)
> > reset_control_get(..., RSTF_SHARED)
> > reset_control_get(..., RSTF_OPTIONAL | RSTF_EXCLUSIVE)
> > reset_control_get(..., RSTF_OPTIONAL | RSTF_SHARED)
> > On the other hand, with that API users could forget the flags or use
> > incompatible combinations, which is impossible with the current API.
This is the age-old 'lines-of-code over {grep,read,maintain}ability'
argument, and I'm afraid to say that this patch, although saves some
lines, the {grep,read,maintain}ability takes too much of a big hit
IMHO.
As a developer, I find it very annoying when I can't directly grep for
functions that have been written in a 'clever' (read 'obfuscated')
way. And since this is a) a header file and b) all of the verboseness
is in a single/central place, it's better left as it is.
I'd entertain the FLAGS idea, since this is very common in the kernel
and users/developers know their way around those already, but MACRO
driven function names, no way!
> If you are unhappy with this patch,
> I will send an alternative one,
> which adds 4 more functions in a stupid and straightforward way.
Yes, 'simple' and straightforward is what we're looking for and is the
way forward.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
