Messages in this thread | | | Subject | Re: [PATCH 1/3] spi: Loongson: Add Loongson 3A+7A SPI controller driver support | From | zhangqing <> | Date | Tue, 24 Nov 2020 09:54:51 +0800 |
| |
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.
| |