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

[Tudor.Ambarus at microchip.com]

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;                  
> }