[PATCH v4 1/1] Initial support for Wiznet W5500

[Quentin Schulz]

Hi Jean-Marie,

On 5/15/25 4:01 PM, Verdun, Jean-Marie wrote:
> 
> 
>>> 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.
> 

Fair enough. You could have the same ternary operator to better match 
what's in the kernel.

Can you also reuse the macros that are in the kernel already? 
W5500_SPI_BLOCK_SELECT, W5500_SPI_READ_CONTROL and 
W5500_SPI_WRITE_CONTROL seems to be of interest?

> 
>>> 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
> 

If you do that, please also update the kernel code to use that :)

>>> 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
> 

Yeah the kernel seems to be using __be16 type for that. And the write 
part seems to be done by hand for the address and data.

>>> 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.
> 

debug() can use the log framework if enabled, but you still need to add 
#define DEBUG wherever you want to build the logging messages. Whereas 
with log_debug you would only need to raise the log level at compile 
time to see the logs. Same, but different.

The benefit of using dev_dbg is when you're having multiple devices from 
the same driver, then you can identify from which one the message is 
being printed. You could have two Wiznet W5500 for example, how do you 
know which one's printing which message?

Up to you,

Cheers,
Quentin