This is a bit strange, the function here is read_cmd_qaddr_qdata which explicitly asks for 4-lines of addr and data. But then this special case is using 1-line for addr and data.

So either:

• add a new function called read_cmd_addr_data which is used for commands like this one
• change this function to be called read_cmd_addr_data and make it select the width of addr and data based on the cmd value, which is what you've done here

The first option feels more correct, because the bus shouldn't care about the values going over it, it should just do as instructed and write the data over the requested number of data lines.

The first option also means softqspi.c and qspi.c don't need to know about (or have a definition of) CMD_RD_SFDP.

I did start going down a path of passing in args to read_cmd_qaddr_qdata to select the required settings... the problem I ran into was that it's not just the number of lines, it's also the dummy cycles. And if both those settings are exposed, the alternate-bytes should be as well arguably. And it all got quite messy and I reverted back to this.

If I ignore the other settings and just base the difference on the number of lines used, passing that in as a function arg is less code space use than duplicating the function and adding the extra entry to the mp_qspi_proto_t.
To be fair these existing functions have dummy/AB settings all hard-coded apparently to suit the commands that just happen to be used by spiflash, so continuing down that path doesn't seem too bad until someone needs them to be more flexible for a new reason.

Oh yeah, I also noticed last time that the new octospi driver in stm only support 1 and 2 line modes currently, auto-selected by which pins are defined...

Yeah I agree it's not all consistent and could be more flexible. You're right that adding a new function takes up more code space, compared to reusing an existing one.

How about calling it read_cmd_addr_data() and passing in the number of lines to use? It could just be an enum with only 2 options to start with, eg:

enum qspi_tranfer_mode {
    QSPI_TRANSFER_CMD_ADDR_DATA,
    QSPI_TRANSFER_CMD_QADDR_QDATA,
};

I'm just finishing up implementing it as an uint8_t lines arg which gets passed 1 or 4 so far.... the enum approach gives better flexibility in future to make a labelled mode that changes different settings.

The enum approach has been added and looks like it works well for me thanks.

This should use mp_printf(MICROPY_ERROR_PRINTER, ...).

Sure, I did initially think that but saw printf had already been used in this file. I'll change it everywhere to use the preferred approach.