Re: [PATCH 4/5] mtd: spi-nor: Add driver for Adaptrum Anarion QSPI controller

[Alexandru Gagniuc]

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