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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3353 bytes --]

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.

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

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

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

> > > +	if (RW_phase == SPI_SLAVE_WRITE) {

> > > +	} else if (RW_phase == SPI_SLAVE_READ) {

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

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

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

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]