Re: [PATCH v7 4/7] can: provide a separate bittiming_const parameter to bittiming functions

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 28.02.2014 14:02, Marc Kleine-Budde wrote:
> On 02/28/2014 01:31 PM, Oliver Hartkopp wrote:
>>
>>
>> On 28.02.2014 11:46, Marc Kleine-Budde wrote:
>>> On 02/28/2014 11:26 AM, Oliver Hartkopp wrote:
>>
>>
>>>>  {
>>>> -	struct can_priv *priv = netdev_priv(dev);
>>>> -	int err;
>>>> +	int err = -EINVAL;
>>>
>>> Keep err uninitialized and make an explicit else err = -EINVAL.... see
>>> below:
>>>
>>
>>
>>>> +	if (!bt->tq) {
>>>> +		/* non-expert mode: Determine bit-timing parameters */
>>>> +		if (bt->bitrate)
>>>
>>> Why not:
>>> 	if (!bt->tq && bt->bitrate) {
>>>
>>>> +			err = can_calc_bittiming(dev, bt, btc);
>>>> +	} else {
>>>
>>> 	} else if (bt->tq && !bt->bitrate) {
>>>
>>>> +		/*
>>>> +		 * expert mode: Check bit-timing parameters and
>>>> +		 * calculate proper brp and a human readable bitrate
>>>> +		 */
>>>> +		if (!bt->bitrate)
>>>> +			err = can_fixup_bittiming(dev, bt, btc);
>>>>  	}
>>> 	} else {
>>> 		err = -EINVAL;
>>> 	}
>>
>> I don't have a strong preference on my implementation.
>>
>> But IMHO your suggested changes only add additional code and additional
>> comparisons in the if statement.
>>
>> I don't see a benefit in your suggestions :-(
> 
> You have 3 seperate if()s and an implicit error value from the
> initialisation. This is IMHO more straight forward:
> 
> 	if (!bt->tq && bt->bitrate)
> 		err = can_calc_bittiming(dev, bt, btc);
> 	else if (bt->tq && !bt->bitrate)
> 		err = can_fixup_bittiming(dev, bt, btc);
> 	else
> 		err = -EINVAL;
> 

Ok - looks good too.

Will change this in the v8 patch series.

Regards,
Oliver