Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936588AbdGTP77 (ORCPT ); Thu, 20 Jul 2017 11:59:59 -0400 Received: from lelnx194.ext.ti.com ([198.47.27.80]:29082 "EHLO lelnx194.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934668AbdGTP75 (ORCPT ); Thu, 20 Jul 2017 11:59:57 -0400 Subject: Re: [PATCH 1/4] can: dev: Add support for limiting configured bitrate To: Sergei Shtylyov , , , , , , , , , , References: <20170719233654.25908-1-fcooper@ti.com> <20170719233654.25908-2-fcooper@ti.com> From: Franklin S Cooper Jr Message-ID: <79d11682-83d8-f56b-4c17-8de1beec70f3@ti.com> Date: Thu, 20 Jul 2017 10:59:36 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [128.247.59.33] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2601 Lines: 86 On 07/20/2017 04:52 AM, Sergei Shtylyov wrote: > Hello! > > On 7/20/2017 2:36 AM, Franklin S Cooper Jr wrote: > >> Various CAN or CAN-FD IP may be able to run at a faster rate than >> what the transceiver the CAN node is connected to. This can lead to >> unexpected errors. However, CAN transceivers typically have fixed >> limitations and provide no means to discover these limitations at >> runtime. Therefore, add support for a fixed-transceiver node that >> can be reused by other CAN peripheral drivers to determine for both >> CAN and CAN-FD what the max bitrate that can be used. If the user >> tries to configure CAN to pass these maximum bitrates it will throw >> an error. >> >> Signed-off-by: Franklin S Cooper Jr >> --- >> drivers/net/can/dev.c | 48 >> ++++++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/can/dev.h | 5 +++++ >> 2 files changed, 53 insertions(+) >> >> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c >> index 365a8cc..fbab87d 100644 >> --- a/drivers/net/can/dev.c >> +++ b/drivers/net/can/dev.c > [...] >> @@ -814,6 +830,38 @@ int open_candev(struct net_device *dev) >> } >> EXPORT_SYMBOL_GPL(open_candev); >> +#ifdef CONFIG_OF >> +void of_transceiver_is_fixed(struct net_device *dev) > > Strange name for a *void* function... Ok I see what you mean since I'm not actually returning anything. I'll go with of_can_transceiver_fixed based on your comment also Oliver's suggestion. > Also, I think 'struct net_device *' variables are typically called > 'ndev'. All other functions within this file uses struct net_device *dev. So I'm just following the style currently used. > >> +{ >> + struct device_node *dn; >> + struct can_priv *priv = netdev_priv(dev); >> + u32 max_frequency; >> + struct device_node *np; >> + >> + np = dev->dev.parent->of_node; >> + >> + /* New binding */ >> + dn = of_get_child_by_name(np, "fixed-transceiver"); >> + if (!dn) >> + return; >> + >> + of_property_read_u32(dn, "max-arbitration-speed", &max_frequency); > > In case this function fails, 'max_frequency' will have no value -- > you'd better initialize it... Thanks for catching this. Will fix. > >> + >> + if (max_frequency > 0) >> + priv->max_trans_arbitration_speed = max_frequency; >> + else >> + priv->max_trans_arbitration_speed = -1; >> + >> + of_property_read_u32(dn, "max-data-speed", &max_frequency); > > Again, when that function fails, the variable will keep the value > from the previous call... Will fix. > > [...] > > MBR, Sergei