Received: by 10.213.65.68 with SMTP id h4csp1067473imn; Thu, 22 Mar 2018 14:35:01 -0700 (PDT) X-Google-Smtp-Source: AG47ELvE+sJkaxXN/16sIIpBwPWTGUi4oa3yeriTB1tGSRlCwCVuXF8l1Tr4O++MDvFfaDIFZ4fU X-Received: by 2002:a17:902:2983:: with SMTP id h3-v6mr6235475plb.80.1521754501497; Thu, 22 Mar 2018 14:35:01 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1521754501; cv=none; d=google.com; s=arc-20160816; b=08G+b33kQl7Il5LkRpSe1X0hWGW62PXiQxDHagVT3dz/qMrkJW1YxxCqBI1kw0pSQC QRCxv5KR/k0ZGj27TMwqZ/swQVTS8jLsIoykmOpjZ2Wg32v5HaZ/VkRDEDgwY8Wn0F9e w+DtFO93Z1ZKLzUIXxt22BFS0hALqwfXXnGCufPJCt/O2DvkOHb+1TaujEKGPRe0oFHW Gq17OlRJtdLRRhVqTVrZWKbbGjwoUy11aGqegfX6z5/xWPSNg0GBugMYh4bDPY0ShZvl COvwfBfgNjGbYk82VlrwydYGZVoDyas9YZ69CYfZqLQplTqzWYXQLgBPrU9k2sw0mzFs OF5w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:cc:to:subject :message-id:date:from:references:in-reply-to:mime-version :dmarc-filter:arc-authentication-results; bh=q/PxVwwR+56ieQdKjxAXeqbdAxQC9GCQ4I9u3T1sMeg=; b=ZVL/Hc1GSVE9jFM3Ghjf0hMufiZ7G0TAgOpv/+HvrUlg+rmtMH/hzyfDlnN/q+iEtr pEUwBFUyAMGg0jwlFZ7x6MeiDatz2TZMra2uMhdqZIM0Oe39rbtrO2b74RffrKrycrko OCGkuMHLFe/q9z0dkbTDw80WepdqiCDEFd+a0dnFJs4Tt6mFoXoQ/vAuJ4Nu0tAXz1yS j3Zi/Sv0cVsnDn7dV1e4e6HD994qLpNlUbzstWrWZ6gDbOYPmEvd3dPBvla2WVP26tm/ O+gqcCLd9sq5KFAT0Ntz0YKW9Y1N3QhPdLjgMGbM3jUkpVuBzZRkxecJ8/yT8ifN099D s5OQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id g8-v6si856057plt.254.2018.03.22.14.34.46; Thu, 22 Mar 2018 14:35:01 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752016AbeCVVdb convert rfc822-to-8bit (ORCPT + 99 others); Thu, 22 Mar 2018 17:33:31 -0400 Received: from mail.kernel.org ([198.145.29.99]:53022 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751595AbeCVVd3 (ORCPT ); Thu, 22 Mar 2018 17:33:29 -0400 Received: from mail-qt0-f182.google.com (mail-qt0-f182.google.com [209.85.216.182]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 655CD2183C; Thu, 22 Mar 2018 21:33:28 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 655CD2183C Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=atull@kernel.org Received: by mail-qt0-f182.google.com with SMTP id j26so10563735qtl.11; Thu, 22 Mar 2018 14:33:28 -0700 (PDT) X-Gm-Message-State: AElRT7GKNiJQPFAsHiWWePg2Uo698UlCLEwvEbBGWwATrXXfusRM6Fjp 3RsJKgYaMdds7RSJ6GNGUJqnnJBlPRZLL30e15o= X-Received: by 10.200.34.80 with SMTP id p16mr37543371qtp.117.1521754407569; Thu, 22 Mar 2018 14:33:27 -0700 (PDT) MIME-Version: 1.0 Received: by 10.200.27.18 with HTTP; Thu, 22 Mar 2018 14:32:47 -0700 (PDT) In-Reply-To: <1521655492.7999.13.camel@perches.com> References: <1521653726-24625-1-git-send-email-p.pisati@gmail.com> <1521653726-24625-3-git-send-email-p.pisati@gmail.com> <1521655492.7999.13.camel@perches.com> From: Alan Tull Date: Thu, 22 Mar 2018 16:32:47 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 2/2] fpga: lattice machxo2: Add Lattice MachXO2 support To: Joe Perches Cc: Paolo Pisati , Moritz Fischer , Rob Herring , Mark Rutland , linux-fpga@vger.kernel.org, "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , linux-kernel Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Mar 21, 2018 at 1:04 PM, Joe Perches wrote: > On Wed, 2018-03-21 at 18:35 +0100, Paolo Pisati wrote: >> This patch adds support to the FPGA manager for programming >> MachXO2 device’s internal flash memory, via slave SPI. > > style trivia: > >> diff --git a/drivers/fpga/machxo2-spi.c b/drivers/fpga/machxo2-spi.c > [] >> +static int get_status(struct spi_device *spi, unsigned long *status) >> +{ >> + struct spi_message msg; >> + struct spi_transfer rx, tx; >> + u8 cmd[] = LSC_READ_STATUS; > > static const u8 cmd[] > here and everywhere else as all the tx_buf assignments > of cmd are to const void * Why *static* const u8 cmd[]? Alan > >> + int ret; >> + >> + memset(&rx, 0, sizeof(rx)); >> + memset(&tx, 0, sizeof(tx)); >> + tx.tx_buf = cmd; > > [] > >> +#ifdef DEBUG >> +static void dump_status_reg(unsigned long *status) >> +{ > > Instead of multiple declarations of dump_status_reg > it's frequently nicer to use a style like > > static void debug_func(args...) > { > #ifdef DEBUG > [code...] > #endif > } > > so if function arguments ever need to be changed > it's only required to be changed in one spot and > not multiply compilation tested with and without > the DEBUG definition > >> + char *ferr; >> + >> + switch (get_err(status)) { >> + case ENOERR: >> + ferr = "No Error"; >> + break; >> + case EID: >> + ferr = "ID ERR"; >> + break; >> + case ECMD: >> + ferr = "CMD ERR"; >> + break; >> + case ECRC: >> + ferr = "CRC ERR"; >> + break; >> + case EPREAM: >> + ferr = "Preamble ERR"; >> + break; >> + case EABRT: >> + ferr = "Abort ERR"; >> + break; >> + case EOVERFL: >> + ferr = "Overflow ERR"; >> + break; >> + case ESDMEOF: >> + ferr = "SDM EOF"; >> + break; >> + default: >> + ferr = "Default switch case"; >> + } > > It's frequently nicer to use a static function > for these enum -> string conversions like: > > static const char *get_err_string(unsigned long err) > { > switch (err) { > case ENOERR: return "No Error"; > case EID: return "ID ERR"; > case ECMD: return "CMD ERR"; > [...] > } > return "default switch case"; > } > >> + pr_debug("machxo2 status: 0x%08lX - done=%d, cfgena=%d, busy=%d, fail=%d, devver=%d, err=%s\n", >> + *status, test_bit(DONE, status), test_bit(ENAB, status), >> + test_bit(BUSY, status), test_bit(FAIL, status), >> + test_bit(DVER, status), ferr); > > So instead of ferr, this could use > get_err_string(*status) > > And please try to keep a consistent alignment for > indentation of multiple line statements > >> +} >> +#else >> +static void dump_status_reg(unsigned long *status) {} >> +#endif >> + >> +static int wait_until_not_busy(struct spi_device *spi) >> +{ >> + unsigned long status; >> + int ret, loop = 0; >> + >> + do { >> + ret = get_status(spi, &status); >> + if (ret) >> + break; >> + if (++loop >= MACHXO2_MAX_BUSY_LOOP) { >> + ret = -EBUSY; >> + break; >> + } >> + } while (test_bit(BUSY, &status)); >> + >> + return ret; >> +} > > Direct returns are OK and would shorten the function > line count. >