Re: [PATCH] reset: generate reset_control_get variants with macro expansion

[Masahiro Yamada]

Hi Philipp,


2016-07-21 17:41 GMT+09:00 Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>:
> Hi Masahiro,
>
> 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.masahiro@xxxxxxxxxxxxx>
>
> Thank you for the patch, but while I'm all for reducing unnecessary
> duplication, I do not like this change for two reasons:
> First, the macro generated API functions can not be found by name using
> tools like ctags or grep anymore.


Linux does this here and there to
not repeat similar function defines.
This is an improvement as we have done from time to time.

For ctags, scripts/tags.sh exists for that purpose, doesn't it?

For example,
commit e0e5070b20e01f0321f97db4e4e174f3f6b49e50
converted func defines and adjusted tags.sh at the same time.



> And secondly, we would now have detailed kerneldoc comments for two
> functions that are never called directly, but the public facing API
> itself is completely without documentation. I wouldn't mind removing
> duplicated documentation paragraphs though, for example by referencing
> reset_control_get_* from the of_reset_control_get_* kerneldoc comments.

Linux is not always kind enough to document every public API.

As you see, we generally do not document
static inline wrappers.

In our case, they are all simply derived from two base functions.
Documenting the two is enough.


I need to expand drivers/usb/host/ehci-platform.c
to support multiple reset lines, so I really want
devm_reset_control_get_optional_shared_by_index() supported.

(So, probably
devm_reset_control_get_exclusive_by_index
devm_reset_control_get_shared_by_index
devm_reset_control_get_optional_exclusive_by_index
as well)

If you are unhappy with this patch,
I will send an alternative one,
which adds 4 more functions in a stupid and straightforward way.


> 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.

Right.

I agree that having dedicated functions is a better idea than flags.

-- 
Best Regards
Masahiro Yamada