From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752209AbcGUMEz (ORCPT ); Thu, 21 Jul 2016 08:04:55 -0400 Received: from mail-wm0-f48.google.com ([74.125.82.48]:38849 "EHLO mail-wm0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751462AbcGUMEx (ORCPT ); Thu, 21 Jul 2016 08:04:53 -0400 Date: Thu, 21 Jul 2016 13:06:00 +0100 From: Lee Jones To: Masahiro Yamada Cc: Philipp Zabel , Linux Kernel Mailing List , Hans de Goede Subject: Re: [PATCH] reset: generate reset_control_get variants with macro expansion Message-ID: <20160721120600.GC14925@dell> References: <1469077523-23163-1-git-send-email-yamada.masahiro@socionext.com> <1469090461.3435.15.camel@pengutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.6.2 (2016-07-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 21 Jul 2016, Masahiro Yamada wrote: > 2016-07-21 17:41 GMT+09:00 Philipp Zabel : > > 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 [...] 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