[PATCH v3 3/8] nand: spi: add basic blocks for infrastructure

[Peter Pan]

Hi Boris,

On Thu, Mar 16, 2017 at 5:55 PM, Boris Brezillon
<boris.brezillon at free-electrons.com> wrote:
> On Thu, 16 Mar 2017 14:47:32 +0800
> Peter Pan <peterpandong at micron.com> wrote:
>
>> +
>> +#define SPINAND_CAP_RD_X1 BIT(0)
>> +#define SPINAND_CAP_RD_X2 BIT(1)
>> +#define SPINAND_CAP_RD_X4 BIT(2)
>> +#define SPINAND_CAP_RD_DUAL BIT(3)
>> +#define SPINAND_CAP_RD_QUAD BIT(4)
>> +#define SPINAND_CAP_WR_X1 BIT(5)
>> +#define SPINAND_CAP_WR_X2 BIT(6)
>> +#define SPINAND_CAP_WR_X4 BIT(7)
>> +#define SPINAND_CAP_WR_DUAL BIT(8)
>> +#define SPINAND_CAP_WR_QUAD BIT(9)
>> +#define SPINAND_CAP_HW_ECC BIT(10)
>
> Empty line please.

Fix this in v4

>
>> +struct spinand_controller {
>> +     struct spinand_controller_ops *ops;
>> +     u32 caps;
>> +     void *priv;
>
> Nope, the ->priv field is a per-device private data, so it should be
> placed in struct spinand_device (see below), otherwise, if you have the
> same controller connected to 2 different chips, each time you call
> spinand_set_controller_data() you will overwrite the ->priv value.
>
> Each spinand_controller should then inherit from struct
> spinand_controller:
>
> struct my_spinand_controller {
>         struct spinand_controller base;
>
>         /* put your SPI NAND controller specific fields here. */
> };

Yes, you are right Boris, I wasn't think so deeply. I will fix this in v4

Thanks
Peter Pan