On 5/26/2016 1:25 PM, Hans de Goede wrote:
> Hi,
>
> On 26-05-16 03:15, John Youn wrote:
>> Prior to commit 6c96f05c8bb8 ("reset: Make [of_]reset_control_get[_foo]
>> functions wrappers"), the optional variants returned -ENOTSUPP when
>> CONFIG_RESET_CONTROLLER was not set. This patch reverts to this
>> behavior. Otherwise those calls will return -EINVAL causing users to
>> think that an error occurred when CONFIG_RESET_CONTROLLER is not set.
>>
>> Fixes: 6c96f05c8bb8 ("reset: Make [of_]reset_control_get[_foo] functions
>> wrappers")
>> Signed-off-by: John Youn <[email protected]>
>> ---
>>
>> Hi Philipp, Hans,
>>
>> The commit referenced above breaks an upcoming patch for the dwc2
>> driver that adds an optional reset control.
>>
>> https://marc.info/?l=linux-usb&m=146161328211584&w=2
>>
>> I've attempted to add the optional variants back the way they were
>> working before. Let me know if I need to do anything else to fix it or
>> if it should be done another way.
>>
>> Regards,
>> John
>
> Hmm, I don't like all the extra code your patch adds just to fix
> a return value...
>
> Looking at the code before my "reset: Make [of_]reset_control_get[_foo]
> functions wrappers" patch, all the dev*_get* functions were
> returning ENOTSUPP except for [devm_]reset_control_get, so following
> your logic we should also change the of_reset_control_get_by_index
> variant to return -ENOTSUP.
>
> Or better, simply make them all return -ENOTSUP, that seems both
> consistent and more KISS to me, this would mean an error code
> change for [devm_]reset_control_get, but will fix all the other
> getters from having a changed error-code, and I would callers
> of [devm_]reset_control_get to not care which error code they
> get, except for -EPROBE_DEFER.
>
> 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
Philipp,
Any suggestions?
Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
