Re: [PATCH 1/2] SPI: Add SPI driver for Sunplus SP7021

On Fri, Nov 05, 2021 at 03:12:32AM +0000, Lh Kuo 郭力豪 wrote:

Please delete unneeded context from mails when replying.  Doing this
makes it much easier to find your reply in the message, helping ensure
it won't be missed by people scrolling through the irrelevant quoted
material.

quoted

quoted

+++ b/drivers/spi/spi-sunplus-sp7021.c

@@ -0,0 +1,1356 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Sunplus SPI controller driver
+ *
+ * Author: Sunplus Technology Co., Ltd.

quoted

Please make the entire comment a C++ one so things look more intentional.

Sorry I don't understand. Is there a explanation?

Make the entire comment block C++ style - //

quoted

If the device has a GPIO chip select it should use the core support for GPIO
chip selects rather than open coding.

Sorry But I didn't find the core function support to use simply. The core function is too complicated for me.

What did you try and in what way was it complicated?  There's lots of
other drivers using this and it's generally resulted in less code in the
drivers so it seems this should be soemthing we can solve.

quoted

quoted

+	if (RW_phase == SPI_SLAVE_WRITE) {

quoted

quoted

+	} else if (RW_phase == SPI_SLAVE_READ) {

quoted

These two cases share no code, they should probably be separate functions
(and what happens if it's an unknown phase?).

The slave mode of SP7021 is only half duplex.

Not sure that really addresses the concern?

quoted

quoted

+	if (pspim->tx_cur_len < data_len) {
+		len_temp = min(pspim->data_unit, data_len);
+		sp7021spi_wb(pspim, len_temp);
+	}

quoted

What if there's more data?

SP7021 only support 16bytes FIFO transfer.

It can transfer more than one FIFO's worth of data though can't it?

quoted

I find it difficult to convince myself that this isn't going to have any overflow
issues, and it'll break operation with anything that does any manipulation of
chip select during the message.  Given that the device uses GPIO chip selects
I'd expect it to just implement transfer_one() and not handle messages at all.
This would greatly simplify the code.

More conditions will be checked in the spi-message function.
In this case, only rx-date is allocated for each transfer of the  message.

Part of the issue with both this and the previous section is code
clarity - it's not just if the code is correct, it's if it's clear that
the code is correct.

quoted

So we are using transfer_one?  In that case I'm very confused why the driver
would be walking a transfer list...

And the spi of SP7021 includes two working modes: spi-master and spi-slave

SP7021 spi-master : full-duplex  FIFO-mode only.
SP7021 spi-slave : helf-duplex  DMA-mode

It seems that linux can contain these two modes in one drive. And this is what I need.
Because many registers are overlapped, if they are used in different drives, 
they will crash if they are declared.

I think the driver needs to be restructured so it's clear which bits
apply to which mode, it's basically two completely separate code paths.
I have to say that it's really very surprising that the two different
modes use such completely different control mechanisms, normally the
differentiation would be more triggered by performance reasons.