>> This is dangerous too. How do I know if the returned data is an error
>> code or actual data?
>> Right now, xfer returns 0 if successful, -1 otherwise. I would suggest
>> return ret only if it is < 0.
>> Or even better, propagate the error code and return data through a
>> pointer as argument of the function?
>> e.g.
>> static int w5500_spi_read(struct udevice *dev, u32 addr, u8 *data)
>> {
>> [...]
>> return xfer(dev, cmd, sizeof(cmd), &data, 1);
>>}
I could do that but that won’t be the same logic as the linux kernel
driver approach which is mixing up ret and data as return value of
this call and many others too.
>> Considering the only difference between w5500_spi_read16 and
>> w5500_spi_read is the 2 or 1 as last argument of xfer() call, you
>> probably want to factor them out into a common function to limit
>> duplicated code.
>> If you have access to the info, I would suggest to NOT use a u8[3] array
>> but create a small struct which specifies what each u8 is for so it's
>> easier to read and debug later on. This applies to all other functions.
I can do that
>> You really need to justify that big of an array, why do you need that?
>> Can't we malloc it based on len?
I switched it to a malloc for next patchset
>> Looks like endianness swap to me? Is this supposed to happen if you have
>> another endianness for the SoC than the one you're currently using to
>> test this?
I don’t have access to anything else other than an ARM SoC to test it. It seems
required to swap the bytes from the SPI read process
>> -ENOMEM?
Done in next patchset
>> Please use log_debug()/dev_dbg instead of debug().
Neil initially asked for debug, I can swap to any, just let me know which one.
Cheers,
Quentin
