lkml.org 
[lkml]   [2020]   [Nov]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/3] spi: Loongson: Add Loongson 3A+7A SPI controller driver support
From
Date
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.

\
 
 \ /
  Last update: 2020-11-24 02:57    [W:0.151 / U:0.156 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site