On 5/30/2016 4:32 AM, Hans de Goede wrote:
> Hi,
>
> On 30-05-16 12:18, Philipp Zabel wrote:
>> Hi,
>>
>> Am Freitag, den 27.05.2016, 09:06 +0200 schrieb Hans de Goede:
>> [...]
>>>>> So IMHO the following change would be a better way to fix this:
>>>>>
>>>>> --- a/include/linux/reset.h
>>>>> +++ b/include/linux/reset.h
>>>>> @@ -65,14 +65,14 @@ static inline struct reset_control *__of_reset_control_get(
>>>>> struct device_node *node,
>>>>> const char *id, int index, int shared)
>>>>> {
>>>>> - return ERR_PTR(-EINVAL);
>>>>> + return ERR_PTR(-ENOTSUPP);
>>>>> }
>>>>>
>>>>> static inline struct reset_control *__devm_reset_control_get(
>>>>> struct device *dev,
>>>>> const char *id, int index, int shared)
>>>>> {
>>>>> - return ERR_PTR(-EINVAL);
>>>>> + return ERR_PTR(-ENOTSUPP);
>>>>> }
>>>>>
>>>>> #endif /* CONFIG_RESET_CONTROLLER */
>>>>
>>>>
>>>> I'm good with this. However, per Philipp on a previous thread, the
>>>> intended behavior is to return -EINVAL for the non-optional functions.
>>>>
>>>> http://marc.info/?l=linux-usb&m=146156945528848&w=2
>>
>> Adding Dinh to Cc: because he wanted this changed from -EINVAL.
>> My point then was that WARN_ON + -EINVAL is indented in this case.
>>
>> Given that the warning backtrace already helps to identify the issue
>> (CONFIG_RESET_CONTROLLER disabled), and that changing the return value
>> to -ENOTSUPP improves consistency with the rest of the API and reduces
>> the amount of inline wrappers, I concur.
>>
>>> As Philipp rightfully points out, calling the non-optional functions
>>> without CONFIG_RESET_CONTROLLER is a no-no, so IMHO we should not have to
>>> care too much about the error code in that case, as long as we return
>>> an error.
>>>
>>> Also note that even before my patch things were already inconsistent
>>> with the of_...get... functions already always returning
>>> -ENOTSUPP when CONFIG_RESET_CONTROLLER was not set, to me it seems
>>> best and also KISS to just return -ENOTSUPP from all get functions
>>> when CONFIG_RESET_CONTROLLER is not set.
>>>
>>> Anyways this is Philipp's call, this is all just my 2 cents.
>>
>> Let the stubs return -ENOTSUPP consistently. A quick grep doesn't show
>> any driver handling -EINVAL explicitly either.
>
> Great.
>
> John Youn, can you submit a patch changing the return of the stubs
> from -EINVAL to -ENOTSUPP please ?
>
Sure. I'll send a patch tomorrow.
Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
