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

[zhangqing <zhangqing@loongson.cn>]

Hi Mark,

Thank you for your suggestion, I will do it and send the v2  in the future.

Thanks,
Qing


On 11/23/2020 09:10 PM, Mark Brown wrote:
> 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.