Re: [PATCH v4 2/3] mtd: spi-nor: Move m25p80 code in spi-nor.c

[Vignesh Raghavendra <vigneshr@ti.com>]

On 05/08/19 3:55 PM, Tudor.Ambarus@microchip.com wrote:
> 
> 
> On 08/01/2019 07:22 PM, Vignesh Raghavendra wrote:
> 
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index e02376e1127b..866962c715b4 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -19,6 +19,7 @@
> 
> cut
> 
>> +static ssize_t spi_nor_spimem_read_data(struct spi_nor *nor, loff_t from,
>> +					size_t len, u8 *buf)
>> +{
>> +	struct spi_mem_op op =
>> +		SPI_MEM_OP(SPI_MEM_OP_CMD(nor->read_opcode, 1),
>> +			   SPI_MEM_OP_ADDR(nor->addr_width, from, 1),
>> +			   SPI_MEM_OP_DUMMY(nor->read_dummy, 1),
>> +			   SPI_MEM_OP_DATA_IN(len, buf, 1));
>> +
>> +	/* get transfer protocols. */
>> +	op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto);
>> +	op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto);
> 
> nit: op.dummy.buswidth = op.addr.buswidth;
> 

Sure, will change.

> cut
> 
>>  
>> @@ -688,6 +1003,16 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>>  	if (nor->erase)
>>  		return nor->erase(nor, addr);
>>  
>> +	if (nor->spimem) {
>> +		struct spi_mem_op op =
>> +			SPI_MEM_OP(SPI_MEM_OP_CMD(nor->erase_opcode, 1),
>> +				   SPI_MEM_OP_ADDR(nor->addr_width, addr, 1),
>> +				   SPI_MEM_OP_NO_DUMMY,
>> +				   SPI_MEM_OP_NO_DATA);
>> +
>> +		return spi_mem_exec_op(nor->spimem, &op);
>> +	}
>> +
> 
> this should be done below in the function, after masking the address.

Nope. It would have been true if addr been sent as part of op.data.buf.out. 
But since addr is being send as part of op.addr.val, spi-mem.c takes care of this for spi_transfer(s)

> 
> You add a double space after return in:
>> +	return  nor->read_reg(nor, SPINOR_OP_RDSR2, sr2, 1);
> 

Ah, Will fix

> There are some checkpatch warnings that we can fix now.
> 

I did see warnings like:
>
>WARNING: line over 80 characters
>#1023: FILE: drivers/mtd/spi-nor/spi-nor.c:2554:
>+				   SPI_MEM_OP_DATA_IN(SPI_NOR_MAX_ID_LEN, id, 1));

I think its okay to overshoot 80 char limit for better readability. 
Let me know if you think otherwise

> ERROR: space required after that ',' (ctx:VxV)
>#1270: FILE: drivers/mtd/spi-nor/spi-nor.c:4846:
>+	{"mx25l25635e"},{"mx66l51235l"},
> 	               ^

This and similar warnings can be fixed, but will throw off alignment wrt previous lines.
Therefore I let them be as is.



> Looks good!
> ta
> 

-- 
Regards
Vignesh