[PATCH v7 1/3] mtd: spi-nor: spansion: Add support for Read/Write Any Register

[Takahiro Kuwano]

Hi Tudor,

Thank you for your suggestions.
I will revise the series

Best Regards,
Takahiro

On 2/1/2022 4:40 PM, Tudor.Ambarus at microchip.com wrote:
> On 2/1/22 09:37, Tudor Ambarus - M18064 wrote:
>> On 1/31/22 13:10, Takahiro Kuwano wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 1/31/2022 4:50 PM, Tudor.Ambarus at microchip.com wrote:
>>>> On 1/28/22 18:20, Pratyush Yadav wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> On 28/01/22 09:43AM, Tudor.Ambarus at microchip.com wrote:
>>>>>> On 7/19/21 11:03, tkuw584924 at gmail.com wrote:
>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>>
>>>>>>> From: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>>>>>>>
>>>>>>> Some of Spansion/Cypress chips support Read/Write Any Register commands.
>>>>>>> These commands are mainly used to write volatile registers.
>>>>>>>
>>>>>>> The Read Any Register instruction (65h) is followed by register address
>>>>>>> and dummy cycles, then the selected register byte is returned.
>>>>>>>
>>>>>>> The Write Any Register instruction (71h) is followed by register address
>>>>>>> and register byte to write.
>>>>>>>
>>>>>>> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
>>>>>>> ---
>>>>>>> Changes in v7:
>>>>>>>   - No change
>>>>>>>
>>>>>>> Changes in v6:
>>>>>>>   - Add helper functions for controller_ops
>>>>>>>   - Add 'reg_addr_width' parameter to spansion_read/write_any_reg()
>>>>>>>   - Remove spi_nor_write_enable() from spansion_write_any_reg() and modified
>>>>>>>     function header comment
>>>>>>>
>>>>>>> Changes in v5:
>>>>>>>   - Fix 'if (ret == 1)' to 'if (ret < 0)' in spansion_read_any_reg()
>>>>>>>
>>>>>>> Changes in v4:
>>>>>>>   - Fix dummy cycle calculation in spansion_read_any_reg()
>>>>>>>   - Modify comment for spansion_write_any_reg()
>>>>>>>
>>>>>>> Changes in v3:
>>>>>>>   - Cleanup implementation
>>>>>>>
>>>>>>>  drivers/mtd/spi-nor/spansion.c | 142 +++++++++++++++++++++++++++++++++
>>>>>>>  1 file changed, 142 insertions(+)
>>>>>>>
>>>>>>> +/**
>>>>>>> + * spansion_read_any_reg() - Read Any Register.
>>>>>>> + * @nor:               pointer to a 'struct spi_nor'
>>>>>>> + * @reg_addr:          register address
>>>>>>> + * @reg_addr_width:    number of address bytes
>>>>>>> + * @reg_dummy:         number of dummy cycles for register read
>>>>>>> + * @reg_val:           pointer to a buffer where the register value is copied
>>>>>>> + *
>>>>>>> + * Return: 0 on success, -errno otherwise.
>>>>>>> + */
>>>>>>> +static int spansion_read_any_reg(struct spi_nor *nor, u32 reg_addr,
>>>>>>> +                                u8 reg_addr_width, u8 reg_dummy, u8 *reg_val)
>>>>>>
>>>>>> how about making this a generic core method? I'm sure there are other SPI NOR
>>>>>> vendors that use registers indexed by address. How about introducing:
>>>>>>
>>>>>> struct spi_nor_reg {
>>>>>>       u32 addr;
>>>>>>       u8 addr_width;
>>>>>>       u8 dummy;
>>>>>>       u8 opcode;
>>>>>>       u8 val;
>>>>>> };
>>>>>>
>>>>>> and passing a pointer to this struct as argument.
>>>>>
>>>>> Wouldn't it be simpler if they fill it in a struct spi_mem_op, and then
>>>>> just pass that in as a "template"?
>>>>>
>>>> sure, sounds fine. I would like a generic method with less function parameters,
>>>> if possible. Let's see what Takahiro will propose.
>>>
>>> So, I would propose...
>>> In core, spi_nor_spimem_{read,write}_reg(struct spi_nor *, struct spi_mem_op *)
>>> that calls spi_nor_spimem_setup() and spi_mem_exec_op().
>>> In vendor specific, spi_mem_op is filled and passed to the core method.
>>>
>>
>> how about the following?
>>
>> static ssize_t spi_nor_spimem_read_reg(struct spi_nor *nor, struct spi_mem_op *op)                    
>> {                                                                                  
>>         bool usebouncebuf;                                                      
>>         ssize_t nbytes;                                                         
>>         int error;      
> s/error/ret
>                                                         
>>                                                                                 
>>         spi_nor_spimem_setup_op(nor, &op, nor->read_proto);
> s/&op/op
>                     
>>                                                                                 
>>         /* convert the dummy cycles to the number of bytes */                   
>>         op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;   
> 
> op->dummy.nbytes and op->dummy.buswidth
>          
>>         if (spi_nor_protocol_is_dtr(nor->read_proto))                           
>>                 op.dummy.nbytes *= 2;   
> op->dummy.nbytes
>                                         
>>                                                                                 
>>         usebouncebuf = spi_nor_spimem_bounce(nor, &op); 
> s/&op/op
>                         
>>                                                                                                                                           
>>         ret = spi_nor_spimem_exec_op(nor, &op);    
> s/&op/op
>                    
>>         if (ret)                                                      
>>                return ret;                                           
>>         nbytes = op.data.nbytes;
> op->data.nbytes
>                                                                                                 
>>                                                                                
>>         if (usebouncebuf && nbytes > 0)                                         
>>                 memcpy(buf, op.data.buf.in, nbytes);    
> op->data.buf.in
>                         
>>                                                                                 
>>         return nbytes;                                                          
>> }  
>>
>> ssize_t spi_nor_read_reg(struct spi_nor *nor, struct spi_mem_op *op)
>> {                                                                               
>>         if (nor->spimem)                                                        
>>                 return spi_nor_spimem_read_reg(nor, from, len, buf);           
>>                                                                                 
>>         return -EOPNOTSUPP;                  
>> } 
>