From mboxrd@z Thu Jan  1 00:00:00 1970
From: Oliver Hartkopp <socketcan@hartkopp.net>
Subject: Re: [PATCH v7 4/7] can: provide a separate bittiming_const parameter
 to bittiming functions
Date: Fri, 28 Feb 2014 13:31:37 +0100
Message-ID: <531081A9.5020107@hartkopp.net>
References: <1393583188-13307-1-git-send-email-socketcan@hartkopp.net> <1393583188-13307-5-git-send-email-socketcan@hartkopp.net> <5310691B.1090507@pengutronix.de>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Return-path: <linux-can-owner@vger.kernel.org>
Received: from mo4-p00-ob.smtp.rzone.de ([81.169.146.218]:56151 "EHLO
	mo4-p00-ob.smtp.rzone.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751514AbaB1Mbn (ORCPT
	<rfc822;linux-can@vger.kernel.org>); Fri, 28 Feb 2014 07:31:43 -0500
In-Reply-To: <5310691B.1090507@pengutronix.de>
Sender: linux-can-owner@vger.kernel.org
List-ID: <linux-can.vger.kernel.org>
To: Marc Kleine-Budde <mkl@pengutronix.de>, linux-can@vger.kernel.org



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 :-(

Regards,
Oliver