Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751699AbdGaWoV (ORCPT ); Mon, 31 Jul 2017 18:44:21 -0400 Received: from mail-wr0-f196.google.com ([209.85.128.196]:34636 "EHLO mail-wr0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751199AbdGaWoT (ORCPT ); Mon, 31 Jul 2017 18:44:19 -0400 Subject: Re: [PATCH 4/5] mtd: spi-nor: Add driver for Adaptrum Anarion QSPI controller To: Alexandru Gagniuc , linux-snps-arc@lists.infradead.org, linux-kernel@vger.kernel.org Cc: David Woodhouse , Brian Norris , Boris Brezillon , Richard Weinberger , Cyrille Pitchen , Rob Herring , Mark Rutland , linux-mtd@lists.infradead.org, devicetree@vger.kernel.org References: <20170728220707.13960-1-alex.g@adaptrum.com> <20170728220707.13960-5-alex.g@adaptrum.com> <135fdf95-1029-2d34-2802-1283a73588e5@gmail.com> <83a4e27a-b96c-0558-fbb5-10e64f9bf59b@adaptrum.com> From: Marek Vasut Message-ID: <1da97744-bc02-420e-d4e9-5ebf331475e8@gmail.com> Date: Tue, 1 Aug 2017 00:43:41 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2746 Lines: 88 On 08/01/2017 12:20 AM, Alexandru Gagniuc wrote: > On 07/31/2017 02:33 PM, Marek Vasut wrote: >> On 07/31/2017 07:17 PM, Alexandru Gagniuc wrote: > > Hi Marek, > > Thank you again for your feedback. I've applied a majority of your > suggestions, and I am very happy with the result. I should have v2 > posted within a day or so. No. You should have v2 out in about a week or so after people have time to review v1 some more. > [snip] >>>>> +/* >>>>> + * This mask does not match reality. Get over it: >>>> >>>> What is this about ? >>> >>> Each stage of the QSPI chain has two registers. The second register has >>> a bitfield which takes in the length of the stage. For example, for >>> DATA2, we can set the length up to 0x4000, but for ADDR2, we can only >>> set a max of 4 bytes. I wrote this comment as a reminder to myself to be >>> careful about using this mask. I'll rephrase the comment for [v2] >> >> Please do. >> > Staged for [PATCH v2] > >>>>> + * DATA2: 0x3fff >>>>> + * CMD2: 0x0003 >>>>> + * ADDR2: 0x0007 >>>>> + * PERF2: 0x0000 >>>>> + * HI_Z: 0x003f >>>>> + * BCNT: 0x0007 >>>>> + */ >>>>> +#define CHAIN_LEN(x) ((x - 1) & ASPI_DATA_LEN_MASK) btw parenthesis around (x) missing, although this is like GEN_MASK() or something here ... >>>>> +struct anarion_qspi { >>>>> + struct spi_nor nor; >>>>> + struct device *dev; >>>>> + uintptr_t regbase; >>>> >>>> Should be void __iomem * I guess ? >>> >>> I chose uintptr_t as opposed to void *, because arithmetic on void * is >>> not valid in C. What is the right answer hen, without risking undefined >>> behavior? >> >> What sort of arithmetic ? It's perfectly valid in general ... > > ISO/IEC 9899:201x, Section 6.5.6, constraint(2) is not met when the one > of the operands to addition is a void pointer. > Section 6.2.5 (19) defines void to be an incomplete type. Is that something new in C 201x draft ? Anyway, this would mean half of the drivers are broken, so I'm not convinced. > [snip] > >>>> Is this stuff below something like ioread32_rep() ? >>>> >>>>> + aspi_write_reg(aspi, ASPI_REG_BYTE_COUNT, sizeof(uint32_t)); >>>>> + while (len >= 4) { >>>>> + data = aspi_read_reg(aspi, ASPI_REG_DATA1); >>>>> + memcpy(buf, &data, sizeof(data)); >>>>> + buf += 4; >>>>> + len -= 4; >>>>> + } >>> >>> That is very similar to ioread32_rep, yes. I kept this as for the >>> reasons outlined above, but changing this to _rep() seems innocent >>> enough. >> >> What reason ? > > Being able to share the code between the different codebases where it is > used. Yes, that argument isn't gonna work, it'd make things impossible to maintain in the kernel. -- Best regards, Marek Vasut