Re: [PATCH] drivers: mtd: m25p80: Add quad read support.

[Sourav Poddar <sourav.poddar@ti.com>]

Hi,
On Sunday 27 October 2013 10:15 PM, Marek Vasut wrote:
> Dear Sourav Poddar,
>
> [...]
>
>> +static int macronix_quad_enable(struct m25p *flash)
>> +{
>> +	int ret, val;
>> +	u8 cmd[2];
>> +	cmd[0] = OPCODE_WRSR;
>> +
>> +	val = read_sr(flash);
>> +	cmd[1] = val | SR_QUAD_EN_MX;
>> +	write_enable(flash);
>> +
>> +	spi_write(flash->spi,&cmd, 2);
>> +
>> +	if (wait_till_ready(flash))
>> +		return 1;
>> +
>> +	ret = read_sr(flash);
> Maybe read_sr() and read_cr() shall be fixed to return retval only and the val
> shall be passed to them as an argument pointer? Aka. ret = read_sr(flash,&val);
>
> That way, this dangerous construct below could become:
>
> if (!(val&  SR_....)) {
> 	dev_err();
> 	ret = -EINVAL;
> }
>
> return ret;
I was trying to work on it and realise, we dont need to pass val directly.
We can continue returning the val and can still cleanup the below code as
u suggetsed above.
if (!(ret & SR_....)) {
     dev_err();
     ret = -EINVAL;
}


> It will also let us prevent mixing of error codes and register values, which is
> pretty ugly practice.
>
>> +	if (!(ret>  0&&  (ret&  SR_QUAD_EN_MX))) {
>> +		dev_err(&flash->spi->dev,
>> +			"Macronix Quad bit not set");
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
> [...]
>
>> +static inline int set_quad_mode(struct m25p *flash, u32 jedec_id)
>> +{
>> +	int status;
>> +
>> +	switch (JEDEC_MFR(jedec_id)) {
>> +	case CFI_MFR_MACRONIX:
>> +		status = macronix_quad_enable(flash);
>> +		if (status) {
>> +			dev_err(&flash->spi->dev,
>> +				"Macronix quad not enable");
> "Macronix quad-read not enabled" would be more sensible, DTTO below.
>
>> +			return -EINVAL;
>> +		}
>> +		return status;
>> +	default:
>> +		status = spansion_quad_enable(flash);
>> +		if (status) {
>> +			dev_err(&flash->spi->dev,
>> +				"Spansion quad not enable");
>> +			return -EINVAL;
>> +		}
>> +		return status;
>> +	}
>> +}
> [...]
>
>> @@ -774,7 +906,7 @@ static const struct spi_device_id m25p_ids[] = {
>>   	{ "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) },
>>   	{ "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, 0) },
>>   	{ "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) },
>> -	{ "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, 0) },
>> +	{ "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, M25P80_QUAD_READ) },
> I'm not convinced enabling 4-bit mode should be hard-coded in the MTD driver.
> There might be a board which uses this chip in 1-bit mode.
>
> We have a setup with Spansion chip here which uses 1-bit addressing in U-Boot,
> but uses 4-bit addressing in Linux. We use a DT property to configure the SPI
> bus width for that and I think that's a way to go. Note that there also are
> chips which use 2-bit wide SPI communication.
>
> [...]
>