Hi Geert,
My 2 cents:
2017-12-04 10:17 GMT+01:00 Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>:
>> EEPROMs using 9 address bits are common (e.g. M95040, 25AA040/25LC040).
>> Do EEPROMs using 17 or 25 address bits, as mentioned in
>> include/linux/spi/eeprom.h, really exist?
>> Or should we just limit it to a single odd value (9 bits)?
>
> At least for the real Atmel parts, only the AT25040 part uses odd (8 +
> 1 bit) addressing.
> AT25M01 uses 3-byte addressing (it needs 17 bits).
>
> So I tend to believe EEPROMs using 16 + 1 or 24 + 1 address bits (with the
> extra bit in the instruction byte) do not exist?
>
I think you are right. Most likely this extra address bit option is
only used for 9 bit addressable chips.
I'm not an expert, but I know only the M95040 chip for which I
originally wrote the patch.
By then I decided to make it a bit broader (so also to be used as
address 17 & 25 bit addressing) but that might
not make any sense indeed.
>> @@ -6,7 +6,9 @@ Required properties:
>> - spi-max-frequency : max spi frequency to use
>> - pagesize : size of the eeprom page
>> - size : total eeprom size in bytes
>> -- address-width : number of address bits (one of 8, 16, or 24)
>> +- address-width : number of address bits (one of 8, 9, 16, 17, 24, or 25).
>> + For odd values, the MSB of the address is sent as bit 3 of the instruction
>> + byte, before the address byte(s).
>
> Alternatively, we can drop the binding change, i.e. keep on using
> address-width = <8> for 512-byte '040...
>
As you also stated before: maybe it is more clear to leave only the
"9" value option documented
here, that looks to me the only valid use case for it.
>> + if (val & 1) {
>> + chip->flags |= EE_INSTR_BIT3_IS_ADDR;
>> + val -= 1;
>> + }
>
> ... and handle it here like:
>
> if (chip->byte_len == 2U << val)
> chip->flags |= EE_INSTR_BIT3_IS_ADDR;
>
> However, that would IMHO be a bit confusing, as the "address-width"
> property is no longer the real address width, but indicates how many bits
> are specified in address bytes sent after the read/write command.
> So "address-bytes" = 1, 2, or 3 would be more correct ;-)
>
> Or deprecate this whole "specify parameters using DT properties" business,
> and derive them from the compatible value. But that would mean adding a
> large and ever growing table to an old driver...
>
> Thoughts?
I'm not a DT expert but to me your first proposal makes the most sense
to me and feels the most intuitive:
I would go for the address-with value 9 option here.
Since we only expect value 9 to be a valid option, maybe you could
rewrite it a bit to explicitly check for value 9:
if (val == 9) {
chip->flags |= EE_INSTR_BIT3_IS_ADDR;
val -= 1;
}
I think this is slightly more readable.
Hope this helps,
Regards,
Ivo Sieben
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
