Re: [PATCH v2 2/6] nand: spi: add basic operations support

[Arnaud Mouiche <arnaud.mouiche@gmail.com>]

On 10/03/2017 02:58, Peter Pan wrote:
> On Fri, Mar 10, 2017 at 1:09 AM, Arnaud Mouiche
> <arnaud.mouiche@gmail.com> wrote:
>>
>> On 09/03/2017 07:02, Boris Brezillon wrote:
>>> [...]
>>>>>
>>>>>> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
>>>>>> index f3d0351..ee447c1 100644
>>>>>> --- a/include/linux/mtd/spinand.h
>>>>>> +++ b/include/linux/mtd/spinand.h
>>>>>> @@ -135,6 +135,8 @@ struct spinand_manufacturer_ops {
>>>>>>         bool(*detect)(struct spinand_device *chip);
>>>>>>         int (*init)(struct spinand_device *chip);
>>>>>>         void (*cleanup)(struct spinand_device *chip);
>>>>>> +     void (*build_column_addr)(struct spinand_device *chip,
>>>>>> +                               struct spinand_op *op, u32 page, u32
>>>>>> column);
>>>>> Okay, I think Arnaud was right, maybe we should have vendor specific
>>>>> ops for basic operations like ->prepare_read/write_op(), instead of
>>>>> having these ->get_dummy() and ->build_column_addr() hooks.
>>>>> Or maybe just a ->prepare_op() hook that can prepare things for any
>>>>> basic operation (read, write, ...).
>>>> I prefer ->prepare_read_op() and ->prepare_write_op. Fix this in v3
>>> I'd like to have Arnaud's feedback on this. Can you wait a bit before
>>> sending a new version?
>>
>> ->prepare_read_op() and ->prepare_write_op
>> looks fine to me.
> Arnaud and Boris,
>
> For prepare_read/write_op, should we put common code in
> spinand_read_from_cache(), spinand_program_data_to_cache() of spinandbase.c
> and then prepare_read/write_op hook just do some fixing ? Code likes this:
>        static int spinand_read_from_cache(struct spinand_device *chip,
>                    u32 page_addr, u32 column, size_t len, u8 *rbuf)
>       {
>             struct spinand_op cmd;
>
>             spinand_op_init(&cmd);
>             cmd.cmd = chip->read_cache_op;
>             cmd.n_addr = 2;
>             cmd.addr[0] = (u8)(column >> 8);
>             cmd.addr[1] = (u8)column;
>             cmd.addr_nbits = spinand_get_address_bits(chip->read_cache_op);
>             cmd.n_rx = len;
>             cmd.rx_buf = rbuf;
>             cmd.data_nbits = spinand_get_data_bits(chip->read_cache_op);
>             if (chip->manufacturer.manu->ops->prepare_op)
>                    chip->manufacturer.manu->ops->prepare_op(chip, &cmd,
>
>        page_addr, column);
>
>            return spinand_exec_cmd(chip, &cmd);
>      }

Personally, I would let ops->prepare_op do all the

            cmd.n_addr = 2;
            cmd.addr[0] = (u8)(column >> 8);
            cmd.addr[1] = (u8)column;
            cmd.addr_nbits = spinand_get_address_bits(chip->read_cache_op);

part unconditionally.
But it is a matter of taste. I would not cost a lot to change the 
behavior if a 2 byte address would not be the main case with future devices.

Arnaud
>
>> FYI: I have prepared a imx6sl board which can run vanilla mtd/master
>> kernels.
>> On this board, I can swap the various spinand samples I have:
>> - Micron (the one supported by those patches)
>> - winbond
>> - macronix
>> - ESMT
>>
>> Looking forward to test v3 ;)
> Arnaud,
>
> Thanks for your help in advance!
>
>
>> Arnaud
>>