On Tue, Jun 12, 2018 at 12:19 PM Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx> wrote: > The struct atmel_ecc_cmd (__packed) is composed of u8 members with only > 2 exceptions, u16 param2 and u16 crc that were written in little endian, > as the device expects. The (u8 *) cast will point to the first member, > which is u8 as well, so we're safe. I2C transfers are done at a per-byte > level, so no problems here either. This is not helpful for the developer, who has to start thinking like the computer before they understand what is going on. It makes the code hard to read IMHO. The i2c transfers are done at a byte level, but the code goes to call cpu_to_le16() to lay out the bytes in the struct in the right way which is confusing if you don't immediately know that the whole struct is going to be serialized. > You are allocating a temporary buffer and duplicate the initialization, > without an obvious benefit. Can you please explain what do want to fix > or what are the potential problems? The buffer is allocated on the stack, which is fine, this is definately not on any hotpath as we're dealing with i2c traffic in the end, but if you are worried about the buffer being reallocated every time we enter the function I can certainly mark it static. It is fundamentally about readability and code centralization, avoiding code duplication I would say. It is pretty hard to see how endianness and marshalling across to the chip happens unless the code dealing with that is collected in one spot. As it is now, that procedure is spread out, and it is hard to figure out how endianness is dealt with. First problem: we have several calls to atmel_ecc_checksum(cmd); spread out all over the place instead of doing the intuitive thing, which is to call that right before we send the command. This is code duplication. Second problem: we have two instances of inlined cmd->param2 = cpu_to_le16(keyid); dealing with endianness instead of doing this as part of marshalling the command. This is also code duplication. Yours, Linus Walleij
