Re: [PATCH 1/3] spi: Loongson: Add Loongson 3A+7A SPI controller driver support

[Mark Brown <broonie@kernel.org>]

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

On Mon, Nov 23, 2020 at 05:19:06PM +0800, Qing Zhang wrote:

> This module is integrated into the Loongson-3A SoC and the LS7A bridge chip.

It looks like this needs quite a bit of update to fit into the SPI
subsystem properly, fortunately most of that is cutting code out of the
driver so you can use core features so it shouldn't be too bad.  There's
also a bunch of pretty minor stylistic issues none of which look too
difficult to address.

> @@ -968,6 +968,12 @@ config SPI_AMD
>  	help
>  	  Enables SPI controller driver for AMD SoC.
>  
> +config SPI_LOONGSON
> +        tristate "Loongson SPI Controller Support"

Please keep Kconfig and Makefile sorted.

> +        depends on CPU_LOONGSON32 || CPU_LOONGSON64

> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Loongson3A+7A SPI driver
> + *

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

> +#include <linux/pci.h>
> +#include <linux/of.h>
> +/*define spi register */
> +#define	SPCR	0x00

Missing blank line.

> +#define	SPSR	0x01
> +#define FIFO	0x02

This indentation is unclear, also all these defines could use some
namespacing to avoid collisions with anything that gets defined in a
header.

> +	hz  = t ? t->speed_hz : spi->max_speed_hz;
> +
> +	if (!hz)
> +		hz = spi->max_speed_hz;

Please write normal conditional statements to improve legibility, and
note that the core will ensure that the transfer always has a speed set.

> +	if ((hz && loongson_spi->hz != hz) || ((spi->mode ^ loongson_spi->mode) & (SPI_CPOL | SPI_CPHA))) {

Please try to keep your lines less than 80 columns (there's not a hard
limit here but it helps legibility).

> +		bit = fls(div) - 1;
> +		if ((1<<bit) == div)

1 << bit

> +static int loongson_spi_setup(struct spi_device *spi)
> +{
> +	struct loongson_spi *loongson_spi;
> +
> +	loongson_spi = spi_master_get_devdata(spi->master);
> +	if (spi->bits_per_word % 8)
> +		return -EINVAL;
> +
> +	if (spi->chip_select >= spi->master->num_chipselect)
> +		return -EINVAL;
> +
> +	loongson_spi_update_state(loongson_spi, spi, NULL);

> +	set_cs(loongson_spi, spi, 1);

The setup() operation shouldn't configure the physical controller state
unless there are separate configuration registers per chip select -
another device could be active when setup() is called.


> +static int loongson_spi_write_read_8bit(struct spi_device *spi,
> +		const u8 **tx_buf, u8 **rx_buf, unsigned int num)
> +{
> +	struct loongson_spi *loongson_spi;
> +	loongson_spi = spi_master_get_devdata(spi->master);
> +
> +	if (tx_buf && *tx_buf) {
> +		loongson_spi_write_reg(loongson_spi, FIFO, *((*tx_buf)++));
> +		while ((loongson_spi_read_reg(loongson_spi, SPSR) & 0x1) == 1);
> +        } else {
> +		loongson_spi_write_reg(loongson_spi, FIFO, 0);
> +		while ((loongson_spi_read_reg(loongson_spi, SPSR) & 0x1) == 1);
> +        }

The indentation is messed up here, looks like you have some kind of
tab/space confusion.

> +	count = xfer->len;
> +
> +	do {
> +		if (loongson_spi_write_read_8bit(spi, &tx, &rx, count) < 0)
> +			goto out;

This is the only caller of _write_read_8bit(), may sa well inline it?

> +static inline int set_cs(struct loongson_spi *loongson_spi, struct spi_device  *spi, int val)

Why is this static inline?  This should be an operation provided to the
SPI core.

> +{
> +	int cs = loongson_spi_read_reg(loongson_spi, SFCS) & ~(0x11 << spi->chip_select);
> +
> +        if (spi->mode  & SPI_CS_HIGH)
> +		val = !val;
> +	loongson_spi_write_reg(loongson_spi, SFCS,
> +                        (val ? (0x11 << spi->chip_select):(0x1 << spi->chip_select)) | cs);

There's mult

> +static void loongson_spi_work(struct work_struct *work)
> +{

Drivers shouldn't be open coding a message queue, implement
transfer_one_message() and let the core handle it for you.

> +static const struct of_device_id loongson_spi_id_table[] = {
> +	{ .compatible = "loongson,loongson-spi", },
> +	{ },
> +};

This is introducing a new DT binding, there should also be a new binding
document added.

> +static int loongson_spi_pci_register(struct pci_dev *pdev,
> +		const struct pci_device_id *ent)
> +{
> +	int ret;
> +	unsigned char v8;

I would expect the PCI device to be a separate module with a dependency
on PCI, I'm kind of surprised that this builds on !PCI systems but I
guess it's possible there's stubs.

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