Re: [PATCH] mtd: spi-nor: core: Introduce spi_nor_abort_octal_dtr()

[Michael Walle <michael@walle.cc>]

Hi,

>> >> To me it seems, that spi_nor_select_{read,pp,erase}() will select
>> >> the wrong commands/proto. So it should be fixed there. Probably
>> >> shared_mask is wrong.
>> > Features in shared_mask have include 2 parts as below.
>> > 1. Flash support the feature (by parse SFDP or ID table flags)
>> > 2. spi host controller support the feature (In
>> > spi_nor_spimem_adjust_hwcaps)
>> >
>> > But I think Octal DTR is a special case because, in addition to the 2
>> > points
>> > mentioned above, there are also other conditions that need to be met.
>> 
>> Correct. And btw we would have the same problem for quad_enable if we
>> wouldn't
>> have a default.
>> 
>> So in spi_nor_default_setup() I'd suggest to add something along
>> 
>> if (!nor->quad_enable)
>>    shared_mask &= ~SNOR_HWCAPS_4_4_4;
> I think this check is unnecessary for quad_enable.
> Because of "spi_nor_sr2_bit1_quad_enable" would be a default
> method for enabling quad mode in spi_nor_init_default_params().

Yes it is at the moment. But if a flash happen to have no
quad_enable we'll have a problem for whatever reason. so for the
sake of completeness we should mask that bit.

>> /* skip octal command mode if we don't have a .octal_enable callback 
>> */
>> if (!nor->octal_enable)
>>    shared_mask &= ~SNOR_HWCAPS_8_8_8;
> Sound great.
> 
> Could I follow the suggestion for preparing v2 patch?

sure.

> BTW, I think discard is better than abort.
> What do you think?

Where do you need that word? You shouln'd need any more
than the code above.

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/