Messages in this thread | | | Subject | Re: [PATCH 4/5] mtd: spi-nor: Add driver for Adaptrum Anarion QSPI controller | From | Alexandru Gagniuc <> | Date | Mon, 31 Jul 2017 15:20:23 -0700 |
| |
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.
[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) >>>> + >>>> +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.
[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.
Alex
| |