On Mon, Aug 29, 2016 at 11:25 PM, Arnd Bergmann <arnd@xxxxxxxx> wrote: > On Monday 29 August 2016, Martin Blumenstingl wrote: >> > and then do a swab32() on them. I suspect these should just be __le32 >> > (and __le16, respectively where needed), using appropriate accessors >> > everywhere that those fields are being read instead of having one function >> > that tries to turn them into cpu-native ordering. >> I'm not sure if we want those fields to be __le32: >> BIT(0) in the eepMisc field indicates whether to interpret the data as >> big or little endian. >> When this bit is set we would want these fields to be __be32 instead - >> so I guess the current implementation is "OK" for this specific >> use-case. > > Ok, I see. It's still confusing because it's different from how other > drivers handle this (case in point: I was very confused by this). lesson learned: specification should state that data is _either_ big or little endian, no mix between these two > Do you see any downsides in changing the code to consistently annotate > the eeprom as either __le or __be (whichever is more common), using > the respective endianess accessors, and then doing the swap if the > data we read is the opposite way? I am assuming you mean leNN_to_cpu() and beNN_to_cpu() here? old logic: - reading data: simply access the struct fields LE system: - LE EEPROM -> no swap will be applied - BE EEPROM -> swab16 / swab32 the fields BE system: - LE EEPROM -> swab16 / swab32 the fields - BE EEPROM -> no swap will be applied new logic (assuming that we went for __le16/__le32 fields): - reading data: use le16_to_cpu and le32_to_cpu for all fields LE system: - LE EEPROM -> no swap will be applied - BE EEPROM -> be16_to_cpu / be32_to_cpu (or swab16 / swab32 as before) BE system: - LE EEPROM -> le16_to_cpu / le32_to_cpu (or swab16 / swab32 as before) - BE EEPROM -> no swap will be applied There is one downside of the "new approach" I can think of: you need to swap the data twice in some cases (BE EEPROM on a BE machine). - first swap while writing the data to __le16/__le32 fields - second swap while reading the data from the __le16/__le32 fields If you forget to swap a field in either place then things will be broken. Maybe someone else wants to state his/her opinion on this - I guess some fresh thoughts could help us a lot! Regards, Martin
