Hi Brown,
Thank you for your suggestions, these are achievable, I will send v3 in
the soon.
Before sending v3, I would like to trouble you to see if this is
correct. It has been tested locally.
On 12/08/2020 09:56 PM, Mark Brown wrote:
On Tue, Dec 08, 2020 at 03:44:24PM +0800, Qing Zhang wrote:
v2:
- keep Kconfig and Makefile sorted
- make the entire comment a C++ one so things look more intentional
You say this but...
+++ b/drivers/spi/spi-ls7a.c
@@ -0,0 +1,324 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Loongson LS7A SPI Controller driver
+ *
+ * Copyright (C) 2020 Loongson Technology Corporation Limited
+ */
...this is still a mix of C and C++ comments?
Replace all with //
+static int set_cs(struct ls7a_spi *ls7a_spi, struct spi_device *spi, int val)
+{
+ int cs = ls7a_spi_read_reg(ls7a_spi, SFCS) & ~(0x11 <<
spi->chip_select);
+
+ if (spi->mode & SPI_CS_HIGH)
+ val = !val;
+ ls7a_spi_write_reg(ls7a_spi, SFCS,
+ (val ? (0x11 << spi->chip_select):(0x1 << spi->chip_select)) |
cs);
+
+ return 0;
+}
Why not just expose this to the core and let it handle things?
Please also write normal conditional statements to improve legibility.
There's quite a lot of coding style issues in this with things like
missing spaces
static void ls7a_spi_set_cs(struct spi_device *spi, bool enable)
{
struct ls7a_spi *ls7a_spi;
int cs = ls7a_spi_read_reg(ls7a_spi, SFCS) & ~(0x11 <<
spi->chip_select));
ls7a_spi = spi_master_get_devdata(spi->master);
if (!!(spi->mode & SPI_CS_HIGH) == enable)
val = (0x11 << spi->chip_select) | cs;
else
val = (0x1 << spi->chip_select) | cs;
ls7a_spi_write_reg(ls7a_spi, SFCS, val);
}
static int ls7a_spi_pci_probe---->
+master->set_cs = ls7a_spi_set_cs;
+ if (t) {
+ hz = t->speed_hz;
+ if (!hz)
+ hz = spi->max_speed_hz;
+ } else
+ hz = spi->max_speed_hz;
If one branch of the conditional has braces please use them on both to
improve legibility.
+static int ls7a_spi_transfer_one_message(struct spi_master *master,
+ struct spi_message *m)
I don't understand why the driver is implementing transfer_one_message()
- it looks like this is just open coding the standard loop that the
framework provides and should just be using transfer_one().
static int ls7a_spi_transfer_one(struct spi_master *master,
struct spi_device *spi,
struct spi_transfer *t)
{
struct ls7a_spi *ls7a_spi;
int param, status;
ls7a_spi = spi_master_get_devdata(master);
spin_lock(&ls7a_spi->lock);
param = ls7a_spi_read_reg(ls7a_spi, PARA);
ls7a_spi_write_reg(ls7a_spi, PARA, param&~1);
spin_unlock(&ls7a_spi->lock);
status = ls7a_spi_do_transfer(ls7a_spi, spi, t);
if(status < 0)
return status;
if(t->len)
r = ls7a_spi_write_read(spi, t);
spin_lock(&ls7a_spi->lock);
ls7a_spi_write_reg(ls7a_spi, PARA, param);
spin_unlock(&ls7a_spi->lock);
return status;
}
static int ls7a_spi_pci_probe---->
- master->transfer_one_message = ls7a_spi_transfer_one_message;
+master->transfer_one = ls7a_spi_transfer_one;
+ r = ls7a_spi_write_read(spi, t);
+ if (r < 0) {
+ status = r;
+ goto error;
+ }
The indentation here isn't following the kernel coding style.
+ master = spi_alloc_master(&pdev->dev, sizeof(struct ls7a_spi));
+ if (!master)
+ return -ENOMEM;
Why not use devm_ here?
- master = spi_alloc_master(&pdev->dev, sizeof(struct ls7a_spi));
error:
- spi_put_master(master);
+ master = devm_spi_alloc_master(&pdev->dev, sizeof(struct ls7a_spi));
+ ret = devm_spi_register_master(dev, master);
+ if (ret)
+ goto err_free_master;
The driver uses devm_spi_register_master() here but...
+static void ls7a_spi_pci_remove(struct pci_dev *pdev)
+{
+ struct spi_master *master = pci_get_drvdata(pdev);
+ struct ls7a_spi *spi;
+
+ spi = spi_master_get_devdata(master);
+ if (!spi)
+ return;
+
+ pci_release_regions(pdev);
...releases the PCI regions in the remove() function before the SPI
controller is freed so the controller could still be active.
static void ls7a_spi_pci_remove(struct pci_dev *pdev)
{
struct spi_master *master = pci_get_drvdata(pdev);
+ spi_unregister_master(master);
pci_release_regions(pdev);
}
Thanks,
-Qing
